Make stylesheet owner node a reference instead of pointer.

Make stylesheet owner node a reference instead of pointer.

The CSSStyleSheet owner node is always non-null when passed in on
sheet creation. Make it a reference in various APIs.

The real change here that triggered this was the realization that
StyleEngine::createSheet is always called with an owner node which
belongs to the very same Document/StyleEngine. So we can turn:

  e->document().styleEngine().addPendingSheet(context);

into:

  addPendingSheet(context);

Also made parseSheet non-static as it needed the StyleEngine pointer
anyway.

R=sashab@chromium.org
BUG=567021
Committed: https://crrev.com/3cf2f58b1be6065c5c8264e60d414ec9531a6449
Cr-Commit-Position: refs/heads/master@{#420018}

[rune, 2016-09-20 14:35:46]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 14:36:05]

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

[rune, 2016-09-20 15:23:40]

ptal

[commit-bot: I haz the power, 2016-09-20 16:16:33]

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

[commit-bot: I haz the power, 2016-09-20 16:16:34]

Dry run: This issue passed the CQ dry run.

[sashab, 2016-09-21 01:33:29]

This looks great, thanks rune!!

I love patches like these. Feel free to do any/all of my feedback in follow-up
patches. Const& changes can be pretty... tempting :)

Also, I'm curious as to how many of these Node&s could also be const Node&s. If
you feel like looking at that too, you could make this patch even better :)

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStyleSheet.cpp:124: ,
m_ownerNode(&ownerNode)
Might be a stretch but... Can we make m_ownerNode a reference too? Or do we need
m_ownerNode to be a Member<> for lifecycle reasons?

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStyleSheetResourceTest.cpp (right):

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStyleSheetResourceTest.cpp:96:
crossfade->loadSubimages(&document());
Could this take a reference too? :) If so maybe add a TODO?

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleElement.cpp:73: void
StyleElement::insertedInto(Element& element, ContainerNode* insertionPoint)
insertionPoint could probably be a ContainerNode& too here and below, happy for
you to do this in a separate patch.

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleElement.cpp:162: ||
csp->allowInlineStyle(document.url(),
element.fastGetAttribute(HTMLNames::nonceAttr), m_startPosition.m_line, text);
Nice additional fix with element.document.url()! :)

[rune, 2016-09-21 07:17:46]

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStyleSheet.cpp (right):

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStyleSheet.cpp:124: ,
m_ownerNode(&ownerNode)
On 2016/09/21 at 01:33:29, sashab wrote:
> Might be a stretch but... Can we make m_ownerNode a reference too? Or do we
need m_ownerNode to be a Member<> for lifecycle reasons?

m_ownerNode is nullptr for @import stylesheets. It will also be null if the
stylesheet is referenced from a script, but the link/style element for the sheet
is removed from the dom.

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSStyleSheetResourceTest.cpp (right):

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSStyleSheetResourceTest.cpp:96:
crossfade->loadSubimages(&document());
On 2016/09/21 at 01:33:29, sashab wrote:
> Could this take a reference too? :) If so maybe add a TODO?

Done.

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/StyleElement.cpp (right):

https://codereview.chromium.org/2354773003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/StyleElement.cpp:73: void
StyleElement::insertedInto(Element& element, ContainerNode* insertionPoint)
On 2016/09/21 at 01:33:29, sashab wrote:
> insertionPoint could probably be a ContainerNode& too here and below, happy
for you to do this in a separate patch.

Yes, most likely.

[rune, 2016-09-21 07:31:15]

On 2016/09/21 at 01:33:29, sashab wrote:
> This looks great, thanks rune!!
> 
> I love patches like these. Feel free to do any/all of my feedback in follow-up
patches. Const& changes can be pretty... tempting :)
> 
> Also, I'm curious as to how many of these Node&s could also be const Node&s.
If you feel like looking at that too, you could make this patch even better :)

That would require a lot of changes. The Node& parameters here are passed into
CSSStyleSheet construction where they're stored non-const, and into the
DocumentOrderedList which offers iteration of non-const objects in its api.

[rune, 2016-09-21 07:45:02]

On 2016/09/21 at 07:31:15, rune wrote:
> On 2016/09/21 at 01:33:29, sashab wrote:
> > This looks great, thanks rune!!
> > 
> > I love patches like these. Feel free to do any/all of my feedback in
follow-up patches. Const& changes can be pretty... tempting :)
> > 
> > Also, I'm curious as to how many of these Node&s could also be const Node&s.
If you feel like looking at that too, you could make this patch even better :)
> 
> That would require a lot of changes. The Node& parameters here are passed into
CSSStyleSheet construction where they're stored non-const, and into the
DocumentOrderedList which offers iteration of non-const objects in its api.

I was able to make one of the parameters const, at least.

[sashab, 2016-09-21 07:54:33]

Lgtm

On 21 Sep 2016 5:45 PM, <rune@opera.com> wrote:

> On 2016/09/21 at 07:31:15, rune wrote:
> > On 2016/09/21 at 01:33:29, sashab wrote:
> > > This looks great, thanks rune!!
> > >
> > > I love patches like these. Feel free to do any/all of my feedback in
> follow-up patches. Const& changes can be pretty... tempting :)
> > >
> > > Also, I'm curious as to how many of these Node&s could also be const
> Node&s.
> If you feel like looking at that too, you could make this patch even
> better :)
> >
> > That would require a lot of changes. The Node& parameters here are
> passed into
> CSSStyleSheet construction where they're stored non-const, and into the
> DocumentOrderedList which offers iteration of non-const objects in its api.
>
> I was able to make one of the parameters const, at least.
>
>
> https://codereview.chromium.org/2354773003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sashab, 2016-09-21 07:54:34]

Lgtm

On 21 Sep 2016 5:45 PM, <rune@opera.com> wrote:

> On 2016/09/21 at 07:31:15, rune wrote:
> > On 2016/09/21 at 01:33:29, sashab wrote:
> > > This looks great, thanks rune!!
> > >
> > > I love patches like these. Feel free to do any/all of my feedback in
> follow-up patches. Const& changes can be pretty... tempting :)
> > >
> > > Also, I'm curious as to how many of these Node&s could also be const
> Node&s.
> If you feel like looking at that too, you could make this patch even
> better :)
> >
> > That would require a lot of changes. The Node& parameters here are
> passed into
> CSSStyleSheet construction where they're stored non-const, and into the
> DocumentOrderedList which offers iteration of non-const objects in its api.
>
> I was able to make one of the parameters const, at least.
>
>
> https://codereview.chromium.org/2354773003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sashab, 2016-09-21 07:55:10]

lgtm

[rune, 2016-09-21 08:00:05]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2016-09-21 08:00:21]

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

[commit-bot: I haz the power, 2016-09-21 09:23:11]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-21 09:26:19]

Description was changed from

==========
Make stylesheet owner node a reference instead of pointer.

The CSSStyleSheet owner node is always non-null when passed in on
sheet creation. Make it a reference in various APIs.

The real change here that triggered this was the realization that
StyleEngine::createSheet is always called with an owner node which
belongs to the very same Document/StyleEngine. So we can turn:

  e->document().styleEngine().addPendingSheet(context);

into:

  addPendingSheet(context);

Also made parseSheet non-static as it needed the StyleEngine pointer
anyway.

R=sashab@chromium.org
BUG=567021
==========

to

==========
Make stylesheet owner node a reference instead of pointer.

The CSSStyleSheet owner node is always non-null when passed in on
sheet creation. Make it a reference in various APIs.

The real change here that triggered this was the realization that
StyleEngine::createSheet is always called with an owner node which
belongs to the very same Document/StyleEngine. So we can turn:

  e->document().styleEngine().addPendingSheet(context);

into:

  addPendingSheet(context);

Also made parseSheet non-static as it needed the StyleEngine pointer
anyway.

R=sashab@chromium.org
BUG=567021

Committed: https://crrev.com/3cf2f58b1be6065c5c8264e60d414ec9531a6449
Cr-Commit-Position: refs/heads/master@{#420018}
==========

[commit-bot: I haz the power, 2016-09-21 09:26:20]

Patchset 3 (id:??) landed as
https://crrev.com/3cf2f58b1be6065c5c8264e60d414ec9531a6449
Cr-Commit-Position: refs/heads/master@{#420018}