Issue 2723793002: De-Element ScriptLoader

Nate Chapin

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-02-28 20:02:46 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/1

3 years, 9 months ago (2017-02-28 20:03:16 UTC) #2

Nate Chapin

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-02-28 21:36:27 UTC) #3

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/40001

3 years, 9 months ago (2017-02-28 21:36:53 UTC) #4

Nate Chapin

japhet@chromium.org changed reviewers: + hiroshige@chromium.org

3 years, 9 months ago (2017-02-28 21:39:28 UTC) #5

Nate Chapin

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h File third_party/WebKit/Source/core/dom/ScriptLoaderClient.h (right): https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h#newcode37 third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:37: class CORE_EXPORT ScriptLoaderClient : public GarbageCollectedMixin { I'm not ...

3 years, 9 months ago (2017-02-28 21:39:29 UTC) #6

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-02-28 23:12:53 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/399598)

3 years, 9 months ago (2017-02-28 23:12:54 UTC) #8

Nate Chapin

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-02-28 23:14:24 UTC) #9

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/60001

3 years, 9 months ago (2017-02-28 23:15:36 UTC) #10

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-01 00:17:33 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/399737)

3 years, 9 months ago (2017-03-01 00:17:34 UTC) #12

Nate Chapin

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-01 00:20:29 UTC) #13

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/60001

3 years, 9 months ago (2017-03-01 00:20:59 UTC) #14

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-01 00:55:59 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/331077)

3 years, 9 months ago (2017-03-01 00:56:00 UTC) #16

hiroshige

Great! https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp#newcode5144 third_party/WebKit/Source/core/dom/Document.cpp:5144: DCHECK(m_currentScriptStack.back().get() == script); nit: DCHECK_EQ https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/Document.h File third_party/WebKit/Source/core/dom/Document.h ...

3 years, 9 months ago (2017-03-01 00:58:15 UTC) #17

Nate Chapin

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-01 21:34:26 UTC) #18

Nate Chapin

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/Document.cpp#newcode5144 third_party/WebKit/Source/core/dom/Document.cpp:5144: DCHECK(m_currentScriptStack.back().get() == script); On 2017/03/01 00:58:14, hiroshige wrote: > ...

3 years, 9 months ago (2017-03-01 21:35:00 UTC) #19

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:5144:
DCHECK(m_currentScriptStack.back().get() == script);
On 2017/03/01 00:58:14, hiroshige wrote:
> nit: DCHECK_EQ

Done.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.h:1028: bool
allowExecutingScripts(Document*);
On 2017/03/01 00:58:14, hiroshige wrote:
> Document::allowExecutingScripts(Document*) looks unclear what it does (e.g.
two
> Documents are involved, what are the two Documents?), so adding a comment
might
> be helpful.
> (Actually I'm still not sure even after I read the implementation of
> allowExecutingScripts() though...what does it intend to do?)

It appears to do 2 things: (1) check that both elementDocument and
contextDocument are attached, (2) check canExecuteScripts() for the
contextDocument. Given that ScriptLoader is the sole caller, I've inlined these
checks to hopefully make them a little clearer.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (left):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoader.cpp:182: if (ScriptLoaderClient*
client = this->client())
On 2017/03/01 00:58:14, hiroshige wrote:
> So client() was always non-null and this if condition was meaningless, right?

I believe so. I guess if buggy code had constructor ScriptLoader with an Element
that was neither HTMLScriptElement nor SVGScriptElement, the null check would
have prevented a crash, but that would be broken for other reasons.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoaderClient.h (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:46: virtual String
sourceAttributeValue() const { return String(); }
On 2017/03/01 00:58:14, hiroshige wrote:
> On 2017/02/28 21:39:29, Nate Chapin wrote:
> > I don't know how good/bad it is to have the default implementations here. At
> the
> > bare minimum, I don't think document() can do anything sane in the base
class.
> 
> I think making these all methods all abstract is better, because, due to the
> large number of virtual functions, it is relatively easy to forget to override
> all of them (and I think we don't expect the default implementation works well
> in the production, except for in tests).

I had been thinking that there are some methods where the default implementation
is benign:
* async/defer, which simply don't exist for SVGScriptElement
* If we do have a non-Element ScriptLoaderClient someday, anything that looks at
attributes can probably safely use the default?

But yeah, I don't know how to balance that with the risk of forgetting to
override something important.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:48: virtual String
typeAttributeValue() const { return String(); }
On 2017/03/01 00:58:14, hiroshige wrote:
> Can we split these methods into subgroups for readability, at least separate
> attributeValue() getters from the others? Sorting attribute getters might also
> be good.

Done.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:62: virtual bool
allowInlineScriptForCSP(const AtomicString& nonce,
On 2017/03/01 00:58:14, hiroshige wrote:
> nit: Can this be a const method?

It calls a CSP function that takes a non-const Element*, so not without doing a
fairly large change to ContentSecurityPolicy to make all its Element* usage
const.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp:34: // ScriptRunner
doesn't appear to depend on it or the associated element.
On 2017/03/01 00:58:14, hiroshige wrote:
> Instead of nullptr ScriptLoaderClient, how about supplying a mock
> ScriptLoaderClient (StrictMock in gmock, i.e. every unintended calls to
> ScriptLoaderClient leads to test failure), in order to keep the invariant that
> "ScriptLoader::m_client is always non-null".
> 
> The mock ScriptLoaderClient will be useful in the future, e.g. when writing
unit
> tests of ScriptLoader.

Done.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/SubresourceIntegrity.h (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/SubresourceIntegrity.h:34: static bool
CheckSubresourceIntegrity(const String& attribute,
On 2017/03/01 00:58:14, hiroshige wrote:
> Naming e.g. |integrityAttribute| is better to explicitly states
> that this is "integrity" attribute.
> 
> An alternative might be to use ScriptLoaderClient here, but optional.

Renamed to |integrityAttribute|.

ScriptLoaderClient won't work, as this is also called by LinkStyle.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/SubresourceIntegrity.h:35: Document&,
On 2017/03/01 00:58:14, hiroshige wrote:
> +A comment that this Document is <script>'s document().
> (Other parameters also lack comments though...)

Done.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/SubresourceIntegrity.h:35: Document&,
On 2017/03/01 00:58:14, hiroshige wrote:
> +A comment that this Document is <script>'s document().
> (Other parameters also lack comments though...)

Slightly more generic comment added, as this can be called for non-<script>

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLOptionElement.cpp (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLOptionElement.cpp:387:
ScriptLoaderClient::fromElementIfPossible((toElement(node)))) {
On 2017/03/01 00:58:14, hiroshige wrote:
> The purpose of this code looks like to determining whether |node| is a
<script>
> element or not, rather than obtaining ScriptLoader/ScriptLoaderClient.
> I expect adding isScriptElement() to Element or somewhere is better (easier to
> read, and hiding the details of ScriptLoader/ScriptLoaderClient from here).

Done.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp
(right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:291:
!ScriptLoaderClient::fromElementIfPossible(toElement(child)));
On 2017/03/01 00:58:14, hiroshige wrote:
> Same as HTMLOptionElement.cpp.

Done.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLConstructionSite.cpp:761:
!ScriptLoaderClient::fromElementIfPossible(element)) {
On 2017/03/01 00:58:15, hiroshige wrote:
> ditto.

Done.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
(right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:65: if
(client->document().frame()) {
On 2017/03/01 00:58:15, hiroshige wrote:
> This changes the behavior: ownerDocument() -> document().

Node::document() and Node::ownerDocument() are equivalent if node->document() !=
node. So long as the Element* in the old code was in fact a HTMLScriptElement or
SVGScriptElement, this isn't a behavior change.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:268:
client->dispatchLoadEvent();
On 2017/03/01 00:58:15, hiroshige wrote:
> Hmm. Conceptually I think we should call scriptLoader->dispatchLoadEvent()
here
> but it slightly changes the behavior, so keeping this line as-is and modifying
> it to call scriptLoader later in another CL is good. (I also found other
slight
> bugs around dispatchLoadEvent() not related to this CL...)

Yeah, I considered calling scriptLoader->dispatchLoadEvent(), but noticed the
slight behavior change, too.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:582:
ScriptLoaderClient::fromElementIfPossible(element);
On 2017/03/01 00:58:15, hiroshige wrote:
> Probably we can remove fromElementIfPossible() call here by changing
> the argument type of requestPendingScript() (and some callers of it) to
> ScriptLoader*.

Possibly, but that requires changing PendingScript's constructor, too. I'm
inclined to leave that for a later PendingScript refactor.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:601: if
(!client)
On 2017/03/01 00:58:15, hiroshige wrote:
> Shouldn't |client| always non-null because we expect |script| to be
> HTMLScriptElement?

It should be, I was just copying the pattern of the early-exit below. But it
looks like that bug is pretty old, and it's quiet possibly bitrotted. Will
DCHECK instead of early exit.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp:1033: if
(ScriptLoaderClient::fromElementIfPossible(newElement))
On 2017/03/01 00:58:15, hiroshige wrote:
> Same as HTMLOptionElement.cpp.

Done.

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/80001

3 years, 9 months ago (2017-03-01 21:35:04 UTC) #20

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-01 23:21:29 UTC) #21

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-01 23:21:30 UTC) #22

hiroshige

hiroshige@chromium.org changed reviewers: + kouhei@chromium.org, sigbjornf@opera.com

3 years, 9 months ago (2017-03-06 22:54:02 UTC) #23

hiroshige

Mostly looks good. Remaining issues from my side are: - Whether ScriptLoaderClient should have default ...

3 years, 9 months ago (2017-03-06 22:54:03 UTC) #24

Mostly looks good. Remaining issues from my side are:
- Whether ScriptLoaderClient should have default implementation, and
- [optional] How should we name ScriptLoaderClient
 
(https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...;
If we don't have any immediate better idea, then the current CL is also OK)

Adding Script-related reviewers.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoaderClient.h (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:46: virtual String
sourceAttributeValue() const { return String(); }
On 2017/03/01 21:34:59, Nate Chapin wrote:
> On 2017/03/01 00:58:14, hiroshige wrote:
> > On 2017/02/28 21:39:29, Nate Chapin wrote:
> > > I don't know how good/bad it is to have the default implementations here.
At
> > the
> > > bare minimum, I don't think document() can do anything sane in the base
> class.
> > 
> > I think making these all methods all abstract is better, because, due to the
> > large number of virtual functions, it is relatively easy to forget to
override
> > all of them (and I think we don't expect the default implementation works
well
> > in the production, except for in tests).
> 
> I had been thinking that there are some methods where the default
implementation
> is benign:
> * async/defer, which simply don't exist for SVGScriptElement
> * If we do have a non-Element ScriptLoaderClient someday, anything that looks
at
> attributes can probably safely use the default?
> 
> But yeah, I don't know how to balance that with the risk of forgetting to
> override something important.

I think it's better to make these abstract, because

> * async/defer, which simply don't exist for SVGScriptElement
This is not trivial and the reason for using (not overriding) the default
implementation in SVGScriptElement is not so clear from the default definition
here.
(Or to be explicitly state this, we might have to add a comment like
"SVGScriptElement doesn't have async/defer" here, which looks layering
violation.)

Implementing these as returning String() in SVGScriptElement (possibly with
comments saying that SVGScriptElement doesn't have these) is clearer.

> * If we do have a non-Element ScriptLoaderClient someday,
> anything that looks at attributes can probably safely use the default?
Even in that case it's safer to enforce the new subclass to implement the
overrides (mostly returning String()/false), because we can explicitly check
which attributes/methods are meaningful for the new subclass or not.
(Probably adding a new ScriptLoaderClient subclass will require substantial work
(not just adding a new subclass but also modifications to other parts of code
and spec), so making the things rigid and explicit will be more helpful than
making the things easier.)

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:62: virtual bool
allowInlineScriptForCSP(const AtomicString& nonce,
On 2017/03/01 21:34:59, Nate Chapin wrote:
> On 2017/03/01 00:58:14, hiroshige wrote:
> > nit: Can this be a const method?
> 
> It calls a CSP function that takes a non-const Element*, so not without doing
a
> fairly large change to ContentSecurityPolicy to make all its Element* usage
> const.

Oh, I see. (and I've just found the ContentSecurityPolicy function eventually
causes event dispatching on the element? and thus probably this cannot be const)

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
(right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:65: if
(client->document().frame()) {
On 2017/03/01 21:34:59, Nate Chapin wrote:
> On 2017/03/01 00:58:15, hiroshige wrote:
> > This changes the behavior: ownerDocument() -> document().
> 
> Node::document() and Node::ownerDocument() are equivalent if node->document()
!=
> node. So long as the Element* in the old code was in fact a HTMLScriptElement
or
> SVGScriptElement, this isn't a behavior change.

Acknowledged.

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:582:
ScriptLoaderClient::fromElementIfPossible(element);
On 2017/03/01 21:35:00, Nate Chapin wrote:
> On 2017/03/01 00:58:15, hiroshige wrote:
> > Probably we can remove fromElementIfPossible() call here by changing
> > the argument type of requestPendingScript() (and some callers of it) to
> > ScriptLoader*.
> 
> Possibly, but that requires changing PendingScript's constructor, too. I'm
> inclined to leave that for a later PendingScript refactor.

Acknowledged.

kouhei (in TOK)

Thanks for working on this! https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h File third_party/WebKit/Source/core/dom/ScriptLoaderClient.h (right): https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h#newcode46 third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:46: virtual String sourceAttributeValue() const ...

3 years, 9 months ago (2017-03-15 03:31:41 UTC) #25

Thanks for working on this!

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoaderClient.h (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:46: virtual String
sourceAttributeValue() const { return String(); }
On 2017/03/06 22:54:03, hiroshige wrote:
> On 2017/03/01 21:34:59, Nate Chapin wrote:
> > On 2017/03/01 00:58:14, hiroshige wrote:
> > > On 2017/02/28 21:39:29, Nate Chapin wrote:
> > > > I don't know how good/bad it is to have the default implementations
here.
> At
> > > the
> > > > bare minimum, I don't think document() can do anything sane in the base
> > class.
> > > 
> > > I think making these all methods all abstract is better, because, due to
the
> > > large number of virtual functions, it is relatively easy to forget to
> override
> > > all of them (and I think we don't expect the default implementation works
> well
> > > in the production, except for in tests).
> > 
> > I had been thinking that there are some methods where the default
> implementation
> > is benign:
> > * async/defer, which simply don't exist for SVGScriptElement
> > * If we do have a non-Element ScriptLoaderClient someday, anything that
looks
> at
> > attributes can probably safely use the default?
> > 
> > But yeah, I don't know how to balance that with the risk of forgetting to
> > override something important.
> 
> I think it's better to make these abstract, because
> 
> > * async/defer, which simply don't exist for SVGScriptElement
> This is not trivial and the reason for using (not overriding) the default
> implementation in SVGScriptElement is not so clear from the default definition
> here.
> (Or to be explicitly state this, we might have to add a comment like
> "SVGScriptElement doesn't have async/defer" here, which looks layering
> violation.)
> 
> Implementing these as returning String() in SVGScriptElement (possibly with
> comments saying that SVGScriptElement doesn't have these) is clearer.
> 
> > * If we do have a non-Element ScriptLoaderClient someday,
> > anything that looks at attributes can probably safely use the default?
> Even in that case it's safer to enforce the new subclass to implement the
> overrides (mostly returning String()/false), because we can explicitly check
> which attributes/methods are meaningful for the new subclass or not.
> (Probably adding a new ScriptLoaderClient subclass will require substantial
work
> (not just adding a new subclass but also modifications to other parts of code
> and spec), so making the things rigid and explicit will be more helpful than
> making the things easier.)

+1 on making this abstract

https://codereview.chromium.org/2723793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
(right):

https://codereview.chromium.org/2723793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:621:
TRACE_EVENT1(
Maybe add a fixme to align ::execute to method name?

Nate Chapin

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-16 18:18:00 UTC) #26

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/100001

3 years, 9 months ago (2017-03-16 18:18:35 UTC) #27

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-16 20:09:47 UTC) #28

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-16 20:09:48 UTC) #29

Nate Chapin

Renamed ScriptElemantBase. PTAL https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h File third_party/WebKit/Source/core/dom/ScriptLoaderClient.h (right): https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Source/core/dom/ScriptLoaderClient.h#newcode46 third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:46: virtual String sourceAttributeValue() const { return ...

3 years, 9 months ago (2017-03-16 20:15:10 UTC) #30

Renamed ScriptElemantBase. PTAL

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptLoaderClient.h (right):

https://codereview.chromium.org/2723793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptLoaderClient.h:46: virtual String
sourceAttributeValue() const { return String(); }
On 2017/03/15 03:31:40, kouhei wrote:
> On 2017/03/06 22:54:03, hiroshige wrote:
> > On 2017/03/01 21:34:59, Nate Chapin wrote:
> > > On 2017/03/01 00:58:14, hiroshige wrote:
> > > > On 2017/02/28 21:39:29, Nate Chapin wrote:
> > > > > I don't know how good/bad it is to have the default implementations
> here.
> > At
> > > > the
> > > > > bare minimum, I don't think document() can do anything sane in the
base
> > > class.
> > > > 
> > > > I think making these all methods all abstract is better, because, due to
> the
> > > > large number of virtual functions, it is relatively easy to forget to
> > override
> > > > all of them (and I think we don't expect the default implementation
works
> > well
> > > > in the production, except for in tests).
> > > 
> > > I had been thinking that there are some methods where the default
> > implementation
> > > is benign:
> > > * async/defer, which simply don't exist for SVGScriptElement
> > > * If we do have a non-Element ScriptLoaderClient someday, anything that
> looks
> > at
> > > attributes can probably safely use the default?
> > > 
> > > But yeah, I don't know how to balance that with the risk of forgetting to
> > > override something important.
> > 
> > I think it's better to make these abstract, because
> > 
> > > * async/defer, which simply don't exist for SVGScriptElement
> > This is not trivial and the reason for using (not overriding) the default
> > implementation in SVGScriptElement is not so clear from the default
definition
> > here.
> > (Or to be explicitly state this, we might have to add a comment like
> > "SVGScriptElement doesn't have async/defer" here, which looks layering
> > violation.)
> > 
> > Implementing these as returning String() in SVGScriptElement (possibly with
> > comments saying that SVGScriptElement doesn't have these) is clearer.
> > 
> > > * If we do have a non-Element ScriptLoaderClient someday,
> > > anything that looks at attributes can probably safely use the default?
> > Even in that case it's safer to enforce the new subclass to implement the
> > overrides (mostly returning String()/false), because we can explicitly check
> > which attributes/methods are meaningful for the new subclass or not.
> > (Probably adding a new ScriptLoaderClient subclass will require substantial
> work
> > (not just adding a new subclass but also modifications to other parts of
code
> > and spec), so making the things rigid and explicit will be more helpful than
> > making the things easier.)
> 
> +1 on making this abstract

Done.

https://codereview.chromium.org/2723793002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp
(right):

https://codereview.chromium.org/2723793002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/parser/HTMLParserScriptRunner.cpp:621:
TRACE_EVENT1(
On 2017/03/15 03:31:40, kouhei wrote:
> Maybe add a fixme to align ::execute to method name?

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/100001

3 years, 9 months ago (2017-03-20 17:44:14 UTC) #33

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-20 19:25:57 UTC) #34

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/412231)

3 years, 9 months ago (2017-03-20 19:25:58 UTC) #35

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2723793002/100001

3 years, 9 months ago (2017-03-20 21:31:41 UTC) #37

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490045453422840, "parent_rev": "812d4a2a764352251eda0a18f34434aefa9f0429", "commit_rev": "d9ab58e7deab459251bbd1d8b1bce3f2d0a4cd16"}

3 years, 9 months ago (2017-03-20 23:12:10 UTC) #38

commit-bot: I haz the power

Description was changed from ========== De-Element ScriptLoader BUG= ========== to ========== De-Element ScriptLoader BUG= Review-Url: ...

3 years, 9 months ago (2017-03-20 23:13:03 UTC) #39

commit-bot: I haz the power

Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d9ab58e7deab459251bbd1d8b1bce3f2d0a4cd16

3 years, 9 months ago (2017-03-20 23:13:08 UTC) #40

hiroshige

3 years, 9 months ago (2017-03-21 03:57:35 UTC) #41

Message was sent while issue was closed.

On 2017/03/20 23:13:08, commit-bot: I haz the power wrote:
> Committed patchset #6 (id:100001) as
>
https://chromium.googlesource.com/chromium/src/+/d9ab58e7deab459251bbd1d8b1bc...

lgtm, thanks!

Issue 2723793002: De-Element ScriptLoader (Closed)

Description

Patch Set 1 #

Patch Set 2 : De-Element ScriptLoader #

Patch Set 3 : De-Element ScriptLoader #

Patch Set 4 : Fix tests #

Patch Set 5 : Address comments #

Patch Set 6 : ScriptLoaderClient->ScriptElementBase, pure virtual interface, add fixme #

Messages