Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2960223002:
Add missing schemes to RecordMainFrameNavigation (Closed)
DescriptionAdd missing schemes to RecordMainFrameNavigation
Previously, urls using less popular schemes were logged as "unknown"
when recording the scheme of the navigation. This change adds seven
less-popular schemes to the list of known schemes.
BUG=737581
Review-Url: https://codereview.chromium.org/2960223002
Cr-Commit-Position: refs/heads/master@{#483449}
Committed: https://chromium.googlesource.com/chromium/src/+/7c451e21433ae47ea740b116d6095bb824781d86
Patch Set 1 #Patch Set 2 : Add Search #
Total comments: 2
Patch Set 3 : Address feedback, add more schemes #
Total comments: 2
Patch Set 4 : Pull dom-distiller scheme from dependency #
Total comments: 2
Patch Set 5 : Fix nit #Patch Set 6 : Fix build.gn and DEPS #
Messages
Total messages: 28 (16 generated)
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add chrome-native to schemes recognized by RecordMainFrameNavigation Previously, urls using the |chrome-native| scheme were logged as unknown when recording the scheme of the navigation. This change adds chrome-native to the list of known schemes. BUG=737581 ========== to ========== Add chrome-native and chrome-search to the list of schemes recognized by RecordMainFrameNavigation. Previously, urls using the |chrome-native| and |chrome-search| schemes were logged as unknown when recording the scheme of the navigation. This change adds these to the list of known schemes. BUG=737581 ==========
Description was changed from ========== Add chrome-native and chrome-search to the list of schemes recognized by RecordMainFrameNavigation. Previously, urls using the |chrome-native| and |chrome-search| schemes were logged as unknown when recording the scheme of the navigation. This change adds these to the list of known schemes. BUG=737581 ========== to ========== Add chrome-native and chrome-search to RecordMainFrameNavigation Previously, urls using the |chrome-native| and |chrome-search| schemes were logged as unknown when recording the scheme of the navigation. This change adds these to the list of known schemes. BUG=737581 ==========
elawrence@chromium.org changed reviewers: + mpearson@chromium.org
PTAL?
lgtm, with an optional comment below --mark https://codereview.chromium.org/2960223002/diff/20001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/2960223002/diff/20001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:28: SCHEME_CHROMENATIVE = 11, optional nit: SCHEME_CHROMENATIVE is hard to parse for me. Consider SCHEME_CHROME_NATIVE analogous comment below
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Add chrome-native and chrome-search to RecordMainFrameNavigation Previously, urls using the |chrome-native| and |chrome-search| schemes were logged as unknown when recording the scheme of the navigation. This change adds these to the list of known schemes. BUG=737581 ========== to ========== Add missing schemes to RecordMainFrameNavigation Previously, urls using less popular schemes were logged as "unknown" when recording the scheme of the navigation. This change adds seven less-popular schemes to the list of known schemes. BUG=737581 ==========
Thanks for the quick look. I changed the naming convention as suggested and added a handful of other schemes that were missing. https://codereview.chromium.org/2960223002/diff/20001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/2960223002/diff/20001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:28: SCHEME_CHROMENATIVE = 11, On 2017/06/28 16:25:19, Mark P wrote: > optional nit: SCHEME_CHROMENATIVE is hard to parse for me. Consider > SCHEME_CHROME_NATIVE > analogous comment below Fixed, thanks.
still lgtm, with one concern --mark https://codereview.chromium.org/2960223002/diff/60001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/2960223002/diff/60001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:50: "chrome-native", nit: you're using raw strings here. Aren't many of these defined as named constants in various places?
elawrence@chromium.org changed reviewers: + wychen@chromium.org
wychen, PTAL at my DEPS change? https://codereview.chromium.org/2960223002/diff/60001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/2960223002/diff/60001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:50: "chrome-native", On 2017/06/28 17:28:42, Mark P wrote: > nit: you're using raw strings here. Aren't many of these defined as named > constants in various places? Yes. Unfortunately, those "various places" are mostly not usable from this component. Most of the constants are defined either in /src/chrome, or defined/redefined in src/content; the latter we cannot depend upon because navigation_metrics isn't a layered component. I've switched to use the constant for chrome-distiller, as that one is defined in a header we can use here.
lgtm. You might want to wrap the CL description.
https://codereview.chromium.org/2960223002/diff/80001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/2960223002/diff/80001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:58: "max", off-topic nit: This last item doesn't seem to be necessary. We could shrink by one and update the assertion below.
Description was changed from ========== Add missing schemes to RecordMainFrameNavigation Previously, urls using less popular schemes were logged as "unknown" when recording the scheme of the navigation. This change adds seven less-popular schemes to the list of known schemes. BUG=737581 ========== to ========== Add missing schemes to RecordMainFrameNavigation Previously, urls using less popular schemes were logged as "unknown" when recording the scheme of the navigation. This change adds seven less-popular schemes to the list of known schemes. BUG=737581 ==========
https://codereview.chromium.org/2960223002/diff/80001/components/navigation_m... File components/navigation_metrics/navigation_metrics.cc (right): https://codereview.chromium.org/2960223002/diff/80001/components/navigation_m... components/navigation_metrics/navigation_metrics.cc:58: "max", On 2017/06/28 23:15:11, wychen wrote: > off-topic nit: This last item doesn't seem to be necessary. We could shrink by > one and update the assertion below. Done.
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, wychen@chromium.org Link to the patchset: https://codereview.chromium.org/2960223002/#ps100001 (title: "Fix nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, wychen@chromium.org Link to the patchset: https://codereview.chromium.org/2960223002/#ps120001 (title: "Fix build.gn and DEPS")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1498759503511270,
"parent_rev": "b40e3694204df33b2ad5f5ed0d3664b3d9b20a3e", "commit_rev":
"7c451e21433ae47ea740b116d6095bb824781d86"}
Message was sent while issue was closed.
Description was changed from ========== Add missing schemes to RecordMainFrameNavigation Previously, urls using less popular schemes were logged as "unknown" when recording the scheme of the navigation. This change adds seven less-popular schemes to the list of known schemes. BUG=737581 ========== to ========== Add missing schemes to RecordMainFrameNavigation Previously, urls using less popular schemes were logged as "unknown" when recording the scheme of the navigation. This change adds seven less-popular schemes to the list of known schemes. BUG=737581 Review-Url: https://codereview.chromium.org/2960223002 Cr-Commit-Position: refs/heads/master@{#483449} Committed: https://chromium.googlesource.com/chromium/src/+/7c451e21433ae47ea740b116d609... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/7c451e21433ae47ea740b116d609... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
