Record UMA when web app manifest contains invalid JSON comments. [chromium/src : main]
François Beaufort (Gerrit)
Attention is currently required from: Dominick Ng, François Beaufort.
François Beaufort uploaded patch set #5 to this change.
Record UMA when web app manifest contains invalid JSON comments.
This CL makes sure a UMA is recorded when web app manifest code contains
invalid JSON comments. It does so by providing an optional boolean
parameter to the Blink JSON parser.
Note that Safari and Firefox don't allow comments in manifest.
Test: Go to https://spiky-pyrite-goose.glitch.me/, and check histograms
in chrome://histograms/Manifest.HasComments
Change-Id: I0cfedf79b7bc009e7cbd4c79dd4e5c94aee4179e
Bug: 1263590, 1264024
---
M third_party/blink/renderer/platform/json/json_parser_test.cc
M third_party/blink/renderer/platform/json/json_parser.cc
M third_party/blink/renderer/platform/json/json_parser.h
M third_party/blink/renderer/modules/manifest/manifest_uma_util.h
M third_party/blink/renderer/modules/manifest/manifest_uma_util.cc
M third_party/blink/renderer/modules/manifest/manifest_parser.cc
M tools/metrics/histograms/metadata/others/histograms.xml
7 files changed, 109 insertions(+), 44 deletions(-)
To view, visit change 3247155. To unsubscribe, or for help writing mail filters, visit settings.
François Beaufort (Gerrit)
Attention is currently required from: Dominick Ng.
1 comment:
File third_party/blink/renderer/modules/manifest/manifest_parser.cc:
Patch Set #3, Line 108:
_, /*strict_mode=*/trueThis is a web-exposed change, so we probably need to treat this as a deprecation (as per https://bug […]
As discussed offline, I've uploaded a new patch. Let me know what you think.
To view, visit change 3247155. To unsubscribe, or for help writing mail filters, visit settings.
François Beaufort (Gerrit)
Attention is currently required from: Matt Giuca, Dominick Ng.
François Beaufort would like Matt Giuca to review this change.
Record UMA when web app manifest contains invalid JSON comments.
This CL makes sure a UMA is recorded when web app manifest code contains
invalid JSON comments. It does so by providing an optional boolean
parameter to the Blink JSON parser.
Note that Safari and Firefox don't allow comments in manifest.
Test: Go to https://spiky-pyrite-goose.glitch.me/, and check histograms
in chrome://histograms/Manifest.HasComments
Change-Id: I0cfedf79b7bc009e7cbd4c79dd4e5c94aee4179e
Bug: 1263590, 1264024
---
M third_party/blink/renderer/platform/json/json_parser_test.cc
M third_party/blink/renderer/platform/json/json_parser.cc
M third_party/blink/renderer/platform/json/json_parser.h
M third_party/blink/renderer/modules/manifest/manifest_uma_util.h
M third_party/blink/renderer/modules/manifest/manifest_uma_util.cc
M third_party/blink/renderer/modules/manifest/manifest_parser.cc
M tools/metrics/histograms/metadata/others/histograms.xml
7 files changed, 109 insertions(+), 44 deletions(-)
To view, visit change 3247155. To unsubscribe, or for help writing mail filters, visit settings.
Dominick Ng (Gerrit)
Attention is currently required from: Matt Giuca, François Beaufort.
4 comments:
Patchset:
Thanks Francois, just one more main thing from me.
File third_party/blink/renderer/modules/manifest/manifest_parser.cc:
Patch Set #3, Line 108:
_, /*strict_mode=*/true
As discussed offline, I've uploaded a new patch. Let me know what you think.
Ack
File third_party/blink/renderer/modules/manifest/manifest_parser.cc:
Patch Set #7, Line 107:
has_comments= false (POD types are not initialized by default)
File third_party/blink/renderer/modules/manifest/manifest_uma_util.cc:
Patch Set #7, Line 67:
UMA_HISTOGRAM_BOOLEANFor deprecation purposes, I think we want a UseCounter, not UMA. That means I think we need to bubble the has_comments up to ManifestManager[1], which should be able to access the UseCounter.
To view, visit change 3247155. To unsubscribe, or for help writing mail filters, visit settings.