Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2971953002:
Skip TabObserver#onUrlUpdated when not in main frame (Closed)
DescriptionSkip TabObserver#onUrlUpdated when not in main frame
onUrlUpdated() should be called when the URL is updated. It doesn't
make sense to call onUrlUpdated() at didFinishNavigation() of the
iframes, since the main URL is not updated.
onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and
excessive calls to onUrlUpdated() translates to excessive Icing calls.
This CL lowers reporting per navigation to <5 roughly.
In UMA, total count and Success bucket count of
Search.IcingContextReportingStatus per navigation should decrease.
BUG=739570
Review-Url: https://codereview.chromium.org/2971953002
Cr-Commit-Position: refs/heads/master@{#485468}
Committed: https://chromium.googlesource.com/chromium/src/+/b1ff788c214ab8c446f9a6df82f333872c6c0da7
Patch Set 1 #
Total comments: 2
Patch Set 2 : merge blocks #Messages
Total messages: 21 (14 generated)
The CQ bit was checked by wychen@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 ========== Skip TabObserver#onUrlUpdated when not in main frame BUG= ========== to ========== Skip TabObserver#onUrlUpdated when not in main frame BUG=739570 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Skip TabObserver#onUrlUpdated when not in main frame BUG=739570 ========== to ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. BUG=739570 ==========
wychen@chromium.org changed reviewers: + yusufo@chromium.org
wychen@chromium.org changed reviewers: + dtrainor@chromium.org
PTAL
https://codereview.chromium.org/2971953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2971953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:236: if (isInMainFrame) { why not do this in the above block?
https://codereview.chromium.org/2971953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java (right): https://codereview.chromium.org/2971953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsObserver.java:236: if (isInMainFrame) { On 2017/07/06 05:20:46, Yusuf wrote: > why not do this in the above block? Done.
Description was changed from ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. BUG=739570 ========== to ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. In UMA, Success bucket count of Search.IcingContextReportingStatus per navigation should decrease. BUG=739570 ==========
Description was changed from ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. In UMA, Success bucket count of Search.IcingContextReportingStatus per navigation should decrease. BUG=739570 ========== to ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. In UMA, total count and Success bucket count of Search.IcingContextReportingStatus per navigation should decrease. BUG=739570 ==========
Description was changed from ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. In UMA, total count and Success bucket count of Search.IcingContextReportingStatus per navigation should decrease. BUG=739570 ========== to ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. This CL lowers reporting per navigation to <5 roughly. In UMA, total count and Success bucket count of Search.IcingContextReportingStatus per navigation should decrease. BUG=739570 ==========
lgtm
lgtm
The CQ bit was checked by wychen@chromium.org
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": 20001, "attempt_start_ts": 1499731146997560,
"parent_rev": "ab6ca8e1793420e9c4479c43057ecb01d7953849", "commit_rev":
"b1ff788c214ab8c446f9a6df82f333872c6c0da7"}
Message was sent while issue was closed.
Description was changed from ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. This CL lowers reporting per navigation to <5 roughly. In UMA, total count and Success bucket count of Search.IcingContextReportingStatus per navigation should decrease. BUG=739570 ========== to ========== Skip TabObserver#onUrlUpdated when not in main frame onUrlUpdated() should be called when the URL is updated. It doesn't make sense to call onUrlUpdated() at didFinishNavigation() of the iframes, since the main URL is not updated. onUrlUpdated() triggers GoogleApiIcingClientImpl#reportContext(), and excessive calls to onUrlUpdated() translates to excessive Icing calls. This CL lowers reporting per navigation to <5 roughly. In UMA, total count and Success bucket count of Search.IcingContextReportingStatus per navigation should decrease. BUG=739570 Review-Url: https://codereview.chromium.org/2971953002 Cr-Commit-Position: refs/heads/master@{#485468} Committed: https://chromium.googlesource.com/chromium/src/+/b1ff788c214ab8c446f9a6df82f3... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b1ff788c214ab8c446f9a6df82f3... |
