Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814853003:
Track whether an element was inserted via document.write (Closed)
|
Created:
4 years, 9 months ago by Charlie Harrison Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, kinuko+watch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrack whether an element was inserted via document.write
This patch adds a member to ScriptLoader which tracks whether elements are created
via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction
service which will learn to preload these scripts when we load their initiator.
BUG=594159
Committed: https://crrev.com/93bafc396d0c61800ad30d8dc569539ca3de7bd9
Cr-Commit-Position: refs/heads/master@{#385218}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : use constructionContextFlags #Patch Set 4 : rebase on #384935 #
Total comments: 3
Patch Set 5 : back to ps2 with some changes #
Messages
Total messages: 26 (8 generated)
Description was changed from ========== Track whether an element was inserted via document.write This patch adds a few members which track whether elements are created via a call to document.write. This will be used to target those elements that are also parser blocking external scripts BUG=594159 ========== to ========== Track whether an element was inserted via document.write This patch adds a member to ScriptLoader which tracks whether elements are created via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction service which will learn to preload these scripts when we load their initiator. BUG=594159 ==========
csharrison@chromium.org changed reviewers: + kouhei@chromium.org, yoav@yoav.ws
PTAL. If there's a better way to do this, please let me know. Right now we lose elements that we can't immediately tokenize.
https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.h:89: bool createdDuringDocumentWrite() { return m_createdDuringDocumentWrite; } Optional Nit: wasCreatedDuringDocumentWrite()? Blink bool getters were expected to start with {is,was,has,have} but not sure after Chromium repo merge... https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLScriptElement.h (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLScriptElement.h:37: static PassRefPtrWillBeRawPtr<HTMLScriptElement> create(Document&, bool wasInsertedByParser, bool alreadyStarted = false, bool createdDuringDocumentWrite = false); Use two value enum here. (non-historic) Blink code is expected to use enum instead of bool. Example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:661: RefPtrWillBeRawPtr<HTMLScriptElement> element = HTMLScriptElement::create(ownerDocumentForCurrentNode(), parserInserted, alreadyStarted, createdDuringDocumentWrite); I think this is ok in this CL, but I'd appreciate if you would do the clean up of {parserInserted,alreadyStarted,createdDuringDocumentWrite} flags. I'm starting to feel guilty about passing multiple bools around in function arguments. I think it would make sense to have a flag enum ElementConstructionContextFlag or something at this point.
https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.h:89: bool createdDuringDocumentWrite() { return m_createdDuringDocumentWrite; } On 2016/03/18 at 02:24:26, kouhei wrote: > Optional Nit: wasCreatedDuringDocumentWrite()? Blink bool getters were expected to start with {is,was,has,have} but not sure after Chromium repo merge... Done. https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLScriptElement.h (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLScriptElement.h:37: static PassRefPtrWillBeRawPtr<HTMLScriptElement> create(Document&, bool wasInsertedByParser, bool alreadyStarted = false, bool createdDuringDocumentWrite = false); On 2016/03/18 at 02:24:26, kouhei wrote: > Use two value enum here. > (non-historic) Blink code is expected to use enum instead of bool. Example: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Are boolean members ok? https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:661: RefPtrWillBeRawPtr<HTMLScriptElement> element = HTMLScriptElement::create(ownerDocumentForCurrentNode(), parserInserted, alreadyStarted, createdDuringDocumentWrite); On 2016/03/18 at 02:24:26, kouhei wrote: > I think this is ok in this CL, but I'd appreciate if you would do the clean up of {parserInserted,alreadyStarted,createdDuringDocumentWrite} flags. > I'm starting to feel guilty about passing multiple bools around in function arguments. > I think it would make sense to have a flag enum ElementConstructionContextFlag or something at this point. I'll do this refactor in this CL, but I'm hitting issues here: https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... How can we change the generated code?
https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLScriptElement.h (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLScriptElement.h:37: static PassRefPtrWillBeRawPtr<HTMLScriptElement> create(Document&, bool wasInsertedByParser, bool alreadyStarted = false, bool createdDuringDocumentWrite = false); On 2016/03/18 14:10:46, csharrison wrote: > On 2016/03/18 at 02:24:26, kouhei wrote: > > Use two value enum here. > > (non-historic) Blink code is expected to use enum instead of bool. Example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Are boolean members ok? I don't see a rule for members, so I guess its ok. https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp (right): https://codereview.chromium.org/1814853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:661: RefPtrWillBeRawPtr<HTMLScriptElement> element = HTMLScriptElement::create(ownerDocumentForCurrentNode(), parserInserted, alreadyStarted, createdDuringDocumentWrite); On 2016/03/18 14:10:47, csharrison wrote: > On 2016/03/18 at 02:24:26, kouhei wrote: > > I think this is ok in this CL, but I'd appreciate if you would do the clean up > of {parserInserted,alreadyStarted,createdDuringDocumentWrite} flags. > > I'm starting to feel guilty about passing multiple bools around in function > arguments. > > I think it would make sense to have a flag enum ElementConstructionContextFlag > or something at this point. > > I'll do this refactor in this CL, but I'm hitting issues here: > https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin... > > How can we change the generated code? See make_element_factory.py https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
I'm putting this on hold while a design from shivanisha@ is formulating. This change may be unnecessary.
Description was changed from ========== Track whether an element was inserted via document.write This patch adds a member to ScriptLoader which tracks whether elements are created via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction service which will learn to preload these scripts when we load their initiator. BUG=594159 ========== to ========== Track whether an element was inserted via document.write This patch adds a member to ScriptLoader which tracks whether elements are created via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction service which will learn to preload these scripts when we load their initiator. BUG=594159 ==========
PTAL. bmcquade@ decided we'd like to incorporate this data into the new DocumentParserTiming class.
On 2016/04/04 at 18:01:33, csharrison wrote: > PTAL. bmcquade@ decided we'd like to incorporate this data into the new DocumentParserTiming class. Kouhei, for more context, the change that will make use of this patch is here: https://codereview.chromium.org/1857903002
bmcquade@chromium.org changed reviewers: + bmcquade@chromium.org
LGTM https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp (right): https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:662: // TODO(csharrison): This logic only works if the tokenizer/parser was not can we open a crbug for this and include a link to it in the comment? i'll do the same in the comments in my patch.
https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.h:40: enum ElementConstructionContextFlags { Thanks for this change! Would you use enum class : int like https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ScriptLoader.h:129: const bool m_wasCreatedDuringDocumentWrite : 1; can we store constructioncontext instead of expanding flags?
On 2016/04/05 13:20:46, kouhei wrote: > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/ScriptLoader.h:40: enum > ElementConstructionContextFlags { > Thanks for this change! > > Would you use enum class : int like > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/ScriptLoader.h:129: const bool > m_wasCreatedDuringDocumentWrite : 1; > can we store constructioncontext instead of expanding flags? By changing to an enum class I realized that this CL will cause more refactoring than I initially though. The enum was being implicitly converted to a bool in the constructors that need |createdByParser|. To properly thread the context flags everywhere, I'll need to update about a dozen more classes. Would you mind if I split this into 2 CLs, so that the refactoring can be done as its own isolated change? That would mean this CL would go back to PS #2, and I'd land the refactor on top of it. WDYT?
On 2016/04/05 at 15:05:56, csharrison wrote: > On 2016/04/05 13:20:46, kouhei wrote: > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/ScriptLoader.h:40: enum > > ElementConstructionContextFlags { > > Thanks for this change! > > > > Would you use enum class : int like > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/ScriptLoader.h:129: const bool > > m_wasCreatedDuringDocumentWrite : 1; > > can we store constructioncontext instead of expanding flags? > > By changing to an enum class I realized that this CL will cause more refactoring than I initially though. The enum was being implicitly converted to a bool in the constructors that need |createdByParser|. > > To properly thread the context flags everywhere, I'll need to update about a dozen more classes. Would you mind if I split this into 2 CLs, so that the refactoring can be done as its own isolated change? That would mean this CL would go back to PS #2, and I'd land the refactor on top of it. > > WDYT? +1 Kouhei if it's ok with you, it'd be really helpful to get the initial version landed so we can begin to collect metrics that depend on this in M51.
On 2016/04/05 15:07:18, Bryan McQuade wrote: > On 2016/04/05 at 15:05:56, csharrison wrote: > > On 2016/04/05 13:20:46, kouhei wrote: > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): > > > > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/dom/ScriptLoader.h:40: enum > > > ElementConstructionContextFlags { > > > Thanks for this change! > > > > > > Would you use enum class : int like > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/dom/ScriptLoader.h:129: const bool > > > m_wasCreatedDuringDocumentWrite : 1; > > > can we store constructioncontext instead of expanding flags? > > > > By changing to an enum class I realized that this CL will cause more > refactoring than I initially though. The enum was being implicitly converted to > a bool in the constructors that need |createdByParser|. > > > > To properly thread the context flags everywhere, I'll need to update about a > dozen more classes. Would you mind if I split this into 2 CLs, so that the > refactoring can be done as its own isolated change? That would mean this CL > would go back to PS #2, and I'd land the refactor on top of it. > > > > WDYT? > > +1 Kouhei if it's ok with you, it'd be really helpful to get the initial version > landed so we can begin to collect metrics that depend on this in M51. lgtm. I like splitting patchecs
On 2016/04/05 at 15:28:59, kouhei wrote: > On 2016/04/05 15:07:18, Bryan McQuade wrote: > > On 2016/04/05 at 15:05:56, csharrison wrote: > > > On 2016/04/05 13:20:46, kouhei wrote: > > > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): > > > > > > > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/dom/ScriptLoader.h:40: enum > > > > ElementConstructionContextFlags { > > > > Thanks for this change! > > > > > > > > Would you use enum class : int like > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > > > > > https://codereview.chromium.org/1814853003/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/dom/ScriptLoader.h:129: const bool > > > > m_wasCreatedDuringDocumentWrite : 1; > > > > can we store constructioncontext instead of expanding flags? > > > > > > By changing to an enum class I realized that this CL will cause more > > refactoring than I initially though. The enum was being implicitly converted to > > a bool in the constructors that need |createdByParser|. > > > > > > To properly thread the context flags everywhere, I'll need to update about a > > dozen more classes. Would you mind if I split this into 2 CLs, so that the > > refactoring can be done as its own isolated change? That would mean this CL > > would go back to PS #2, and I'd land the refactor on top of it. > > > > > > WDYT? > > > > +1 Kouhei if it's ok with you, it'd be really helpful to get the initial version > > landed so we can begin to collect metrics that depend on this in M51. > > lgtm. I like splitting patchecs Great, thanks! Charles, one small change request for patch 2: let's keep the rename from bool createdDuringDocumentWrite() to bool wasCreatedDuringDocumentWrite() in the initial patch you land.
Done.
lgtm
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/1814853003/#ps80001 (title: "back to ps2 with some changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814853003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814853003/80001
Message was sent while issue was closed.
Description was changed from ========== Track whether an element was inserted via document.write This patch adds a member to ScriptLoader which tracks whether elements are created via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction service which will learn to preload these scripts when we load their initiator. BUG=594159 ========== to ========== Track whether an element was inserted via document.write This patch adds a member to ScriptLoader which tracks whether elements are created via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction service which will learn to preload these scripts when we load their initiator. BUG=594159 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Track whether an element was inserted via document.write This patch adds a member to ScriptLoader which tracks whether elements are created via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction service which will learn to preload these scripts when we load their initiator. BUG=594159 ========== to ========== Track whether an element was inserted via document.write This patch adds a member to ScriptLoader which tracks whether elements are created via a call to document.write. These scripts are going to be parser blocking. The plan is to use this data for metrics tracking, as well as driving a prediction service which will learn to preload these scripts when we load their initiator. BUG=594159 Committed: https://crrev.com/93bafc396d0c61800ad30d8dc569539ca3de7bd9 Cr-Commit-Position: refs/heads/master@{#385218} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/93bafc396d0c61800ad30d8dc569539ca3de7bd9 Cr-Commit-Position: refs/heads/master@{#385218} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
