Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1303833002:
Add WebPageImportanceSignals::issuedNonGetFetchFromScript() (Closed)
|
Created:
5 years, 4 months ago by kouhei (in TOK) Modified:
5 years, 2 months ago CC:
blink-reviews, dglazkov+blink, tyoshino+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd WebPageImportanceSignals::issuedNonGetFetchFromScript()
This CL adds "issuedNonGetFetchFromScript" flag to WebPageImportanceSignals.
From this signal, we assume that the page state is not simply discardable,
as it has issued some dynamic resource request with side-effects.
BUG=520838
Committed: https://crrev.com/38bc2c7b6ca3fa40b63612bb015f11f961900a15
Cr-Commit-Position: refs/heads/master@{#351020}
Patch Set 1 #
Total comments: 1
Patch Set 2 : hook DocumentThreadableLoader #
Total comments: 10
Patch Set 3 : histograms.xml / tyoshino-san review #Patch Set 4 : update base-url #Patch Set 5 : add missing } #
Total comments: 1
Patch Set 6 : rebase fixes #
Total comments: 6
Patch Set 7 : tyoshino-san review #
Total comments: 9
Patch Set 8 : rebase / tkent-san review #Patch Set 9 : m_document\.page() not ->page() #
Messages
Total messages: 38 (11 generated)
kouhei@chromium.org changed reviewers: + tyoshino@chromium.org, tzik@chromium.org, yhirano@chromium.org
I think fetch() and XHR should be treated equally.
https://codereview.chromium.org/1303833002/diff/1/Source/core/xmlhttprequest/... File Source/core/xmlhttprequest/XMLHttpRequest.cpp (right): https://codereview.chromium.org/1303833002/diff/1/Source/core/xmlhttprequest/... Source/core/xmlhttprequest/XMLHttpRequest.cpp:576: if (method != "GET") { Is it better to place the block on send function?
tyoshino: Would you take a look?
https://codereview.chromium.org/1303833002/diff/20001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1303833002/diff/20001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:165: document.frame()->page()->chromeClient().observedNonGetFetchFromScript(); use m_document for consistency with other users add if-condition to check non-nullness of frame() and page(). https://codereview.chromium.org/1303833002/diff/20001/Source/core/page/Chrome... File Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/1303833002/diff/20001/Source/core/page/Chrome... Source/core/page/ChromeClient.h:67: class WebPageImportanceSignals; remove https://codereview.chromium.org/1303833002/diff/20001/Source/core/page/Chrome... Source/core/page/ChromeClient.h:255: virtual void observedNonGetFetchFromScript() const { } write a note to say that this doesn't capture all fetch. https://codereview.chromium.org/1303833002/diff/20001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1303833002/diff/20001/Source/web/ChromeClient... Source/web/ChromeClientImpl.cpp:107: WebPageImportanceSignals.h https://codereview.chromium.org/1303833002/diff/20001/Source/web/WebPageImpor... File Source/web/WebPageImportanceSignals.cpp (right): https://codereview.chromium.org/1303833002/diff/20001/Source/web/WebPageImpor... Source/web/WebPageImportanceSignals.cpp:12: #include "core/page/ChromeClient.h" remove
kouhei@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: Would you take a look at the UMA xml? https://codereview.chromium.org/1303833002/diff/20001/Source/core/loader/Docu... File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1303833002/diff/20001/Source/core/loader/Docu... Source/core/loader/DocumentThreadableLoader.cpp:165: document.frame()->page()->chromeClient().observedNonGetFetchFromScript(); On 2015/09/24 08:14:11, tyoshino wrote: > use m_document for consistency with other users > > add if-condition to check non-nullness of frame() and page(). Done. https://codereview.chromium.org/1303833002/diff/20001/Source/core/page/Chrome... File Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/1303833002/diff/20001/Source/core/page/Chrome... Source/core/page/ChromeClient.h:67: class WebPageImportanceSignals; On 2015/09/24 08:14:11, tyoshino wrote: > remove Done. https://codereview.chromium.org/1303833002/diff/20001/Source/core/page/Chrome... Source/core/page/ChromeClient.h:255: virtual void observedNonGetFetchFromScript() const { } On 2015/09/24 08:14:11, tyoshino wrote: > write a note to say that this doesn't capture all fetch. Done. https://codereview.chromium.org/1303833002/diff/20001/Source/web/ChromeClient... File Source/web/ChromeClientImpl.cpp (right): https://codereview.chromium.org/1303833002/diff/20001/Source/web/ChromeClient... Source/web/ChromeClientImpl.cpp:107: On 2015/09/24 08:14:11, tyoshino wrote: > WebPageImportanceSignals.h Done. https://codereview.chromium.org/1303833002/diff/20001/Source/web/WebPageImpor... File Source/web/WebPageImportanceSignals.cpp (right): https://codereview.chromium.org/1303833002/diff/20001/Source/web/WebPageImpor... Source/web/WebPageImportanceSignals.cpp:12: #include "core/page/ChromeClient.h" On 2015/09/24 08:14:11, tyoshino wrote: > remove Done.
Sorry, patch is buggy from failed rebase. Will update shortly.
https://codereview.chromium.org/1303833002/diff/40002/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPageImportanceSignals.cpp (right): https://codereview.chromium.org/1303833002/diff/40002/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPageImportanceSignals.cpp:31: void WebPageImportanceSignals::setHadFormInteraction() s/setHadFormInteraction/setIssuedNonGetFetchFromScript/?
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
Ok. PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303833002/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303833002/90001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/ChromeClient.h:255: // Called when observed XHR, fetch, and other fetch request with non-GET method is initiated from javascript. At this time, it is not guaranteed that this is comprehensive. wrap comments to 80 cols https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPageImportanceSignals.cpp (right): https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPageImportanceSignals.cpp:15: m_hadFormInteraction = false; reset m_issuedNonGetFetchFromScrip https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPageImportanceSignals.cpp:29: m_issuedNonGetFetchFromScript = false; hmm, true?
https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/ChromeClient.h:255: // Called when observed XHR, fetch, and other fetch request with non-GET method is initiated from javascript. At this time, it is not guaranteed that this is comprehensive. On 2015/09/25 06:22:44, tyoshino wrote: > wrap comments to 80 cols Done. https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebPageImportanceSignals.cpp (right): https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPageImportanceSignals.cpp:15: m_hadFormInteraction = false; On 2015/09/25 06:22:45, tyoshino wrote: > reset m_issuedNonGetFetchFromScrip Done. https://codereview.chromium.org/1303833002/diff/90001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebPageImportanceSignals.cpp:29: m_issuedNonGetFetchFromScript = false; On 2015/09/25 06:22:44, tyoshino wrote: > hmm, true? Done.
lgtm
lgtm
kouhei@chromium.org changed reviewers: + tkent@chromium.org
tkent: Would you do a OWNER review?
https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:168: // we use this chance to update PageImportanceSignals. DocumentThreadableLoader should not have knowledge of PageImportanceSignals. - Passing httpMethod() to observedNonGetFetchFromScript(), and - Move this comment to ChromeClientImpl might be better. But the code logic depends on the details of ThreadableLoader. So the current structure is also acceptable. https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:180: if (request.httpMethod() != "GET") { Is it safe to assume httpMethod() is always upper-cased? https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:181: if (LocalFrame* frame = m_document.frame()) { m_document.page() is simpler. https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/ChromeClient.h:258: virtual void observedNonGetFetchFromScript() const {} In Blink convention, such notification function should be named as "didObserveNonGetFetchFromScript"
https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:168: // we use this chance to update PageImportanceSignals. On 2015/09/27 23:40:51, tkent wrote: > DocumentThreadableLoader should not have knowledge of PageImportanceSignals. > - Passing httpMethod() to observedNonGetFetchFromScript(), and > - Move this comment to ChromeClientImpl > might be better. But the code logic depends on the details of ThreadableLoader. > So the current structure is also acceptable. Thanks for your suggestion. I thought over this again, but I prefer to keep this comment/code here: - The ThreadableLoader design is what is important here, so the comment would be more benefitial when written here. - I updated the comment slightly to remove explicit reference to PageImportanceSignals. https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:180: if (request.httpMethod() != "GET") { On 2015/09/27 23:40:52, tkent wrote: > Is it safe to assume httpMethod() is always upper-cased? Yes. Follows other use cases: https://code.google.com/p/chromium/codesearch#search/&q=%22httpMethod()%20==%... https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:181: if (LocalFrame* frame = m_document.frame()) { On 2015/09/27 23:40:51, tkent wrote: > m_document.page() is simpler. Done. https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/ChromeClient.h (right): https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/ChromeClient.h:258: virtual void observedNonGetFetchFromScript() const {} On 2015/09/27 23:40:52, tkent wrote: > In Blink convention, such notification function should be named as > "didObserveNonGetFetchFromScript" Done.
lgtm https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/1303833002/diff/110001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp:180: if (request.httpMethod() != "GET") { On 2015/09/28 at 00:42:44, kouhei (catching-up) wrote: > On 2015/09/27 23:40:52, tkent wrote: > > Is it safe to assume httpMethod() is always upper-cased? > > Yes. Follows other use cases: https://code.google.com/p/chromium/codesearch#search/&q=%22httpMethod()%20==%... Someone should document it in ResourceRequest.h.
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org, tyoshino@chromium.org, asvitkine@chromium.org, tkent@chromium.org Link to the patchset: https://codereview.chromium.org/1303833002/#ps150001 (title: "m_document\.page() not ->page()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303833002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303833002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303833002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303833002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kouhei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1303833002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1303833002/150001
Message was sent while issue was closed.
Committed patchset #9 (id:150001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/38bc2c7b6ca3fa40b63612bb015f11f961900a15 Cr-Commit-Position: refs/heads/master@{#351020} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
