Change CSSParserContext to have a Document handle (vs UseCounter).

Change CSSParserContext to have a Document handle (vs UseCounter).

This is a refactor only, no functionality should change. This gives
CSSParserContext a WeakMember reference to a Document instead of a raw
pointer to a UseCounter. Also gave CSSParserContext a count method so
clients aren't forced to check for isUseCounterRecordingEnabled every
time they want to count something. Also moved some getter methods around
because some of them weren't necessary any more.

BUG=668251
Review-Url: https://codereview.chromium.org/2671173002
Cr-Commit-Position: refs/heads/master@{#449449}
Committed: https://chromium.googlesource.com/chromium/src/+/341a3f42f1319415886f11e12a3f1b84c8dd182d

[Bret, 2017-02-05 03:15:56]

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

[commit-bot: I haz the power, 2017-02-05 03:16:10]

Dry run: 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, 2017-02-05 03:27:21]

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

[commit-bot: I haz the power, 2017-02-05 03:27:22]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Bret, 2017-02-07 23:52:25]

Description was changed from

==========
Change CSSParserContext to have a Document handle (vs UseCounter).

This is a refactor only, no functionality should change. This gives
CSSParserContext a WeakMember reference to a Document instead of a raw
pointer to a UseCounter. Also gave CSSParserContext a count method so
clients aren't forced to check for isUseCounterRecordingEnabled every
time they want to count something. Also moved some getter methods around
because some of them weren't necessary any more.

BUG=668251
==========

to

==========
Change CSSParserContext to have a Document handle (vs UseCounter).

This is a refactor only, no functionality should change. This gives
CSSParserContext a WeakMember reference to a Document instead of a raw
pointer to a UseCounter. Also gave CSSParserContext a count method so
clients aren't forced to check for isUseCounterRecordingEnabled every
time they want to count something. Also moved some getter methods around
because some of them weren't necessary any more.

BUG=668251
==========

[Bret, 2017-02-07 23:52:25]

bsep@chromium.org changed reviewers:
+ haraken@chromium.org

[Bret, 2017-02-07 23:57:59]

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

[commit-bot: I haz the power, 2017-02-07 23:58:45]

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

[haraken, 2017-02-08 01:40:01]

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.h (right):

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserContext.h:115:
WeakMember<const Document> m_useCounterDocument;

Does this need to be weak? In other words, can CSSParserContext outlive the
Document?

If no, we should just use a Member.

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserContext.h:115:
WeakMember<const Document> m_useCounterDocument;

Also I'd rename m_useCounterDocument to m_document, since it's just a document
associated with the CSSParserContext.

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1466: static const Document*
singleOwnerDocument(const CSSStyleSheet*);

I'd move these methods to StyleSheetContents (or somewhere in core/style/).

[commit-bot: I haz the power, 2017-02-08 02:14:37]

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

[commit-bot: I haz the power, 2017-02-08 02:14:39]

Dry run: This issue passed the CQ dry run.

[Bret, 2017-02-09 01:53:34]

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.h (right):

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserContext.h:115:
WeakMember<const Document> m_useCounterDocument;
On 2017/02/08 01:40:00, haraken wrote:
> 
> Also I'd rename m_useCounterDocument to m_document, since it's just a document
> associated with the CSSParserContext.

Rename done.

I believe it has to be a WeakMember. I originally made it a Member and
CSSLazyParsingTest.ChangeDocuments failed because m_cachedContents was holding
onto the CSSParserContext handle, which caused the Document to not be collected,
which caused CSSLazyParsingState's weak Document handle to still be valid, which
caused it to not refresh it from m_owningContents, which caused the
isDocumentHandleEqual call to return true, which caused it to use the old
UseCounter instance. Maybe something there can be simplified but I don't have
enough knowledge to know where.

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.h:1466: static const Document*
singleOwnerDocument(const CSSStyleSheet*);
On 2017/02/08 01:40:00, haraken wrote:
> 
> I'd move these methods to StyleSheetContents (or somewhere in core/style/).

Done. I moved these into their own class files (StyleSheetContents and
CSSStyleSheet respectively).

[haraken, 2017-02-09 02:01:42]

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.h (right):

https://codereview.chromium.org/2671173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserContext.h:115:
WeakMember<const Document> m_useCounterDocument;
On 2017/02/09 01:53:33, Bret Sepulveda wrote:
> On 2017/02/08 01:40:00, haraken wrote:
> > 
> > Also I'd rename m_useCounterDocument to m_document, since it's just a
document
> > associated with the CSSParserContext.
> 
> Rename done.
> 
> I believe it has to be a WeakMember. I originally made it a Member and
> CSSLazyParsingTest.ChangeDocuments failed because m_cachedContents was holding
> onto the CSSParserContext handle, which caused the Document to not be
collected,
> which caused CSSLazyParsingState's weak Document handle to still be valid,
which
> caused it to not refresh it from m_owningContents, which caused the
> isDocumentHandleEqual call to return true, which caused it to use the old
> UseCounter instance. Maybe something there can be simplified but I don't have
> enough knowledge to know where.

Yeah, I think we should look into what's going on there and clean up.

This CL LGTM though.

https://codereview.chromium.org/2671173002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/2671173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:45: //
FIXME(https://crbug.com/242125): We may want to handle stylesheets that

TODO

[Bret, 2017-02-09 02:08:19]

https://codereview.chromium.org/2671173002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/2671173002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:45: //
FIXME(https://crbug.com/242125): We may want to handle stylesheets that
On 2017/02/09 02:01:42, haraken wrote:
> 
> TODO

Done.

[Bret, 2017-02-09 02:08:23]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-02-09 02:08:24]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2671173002/#ps60001
(title: "fix todo style")

[commit-bot: I haz the power, 2017-02-09 02:08:54]

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, 2017-02-09 04:23:01]

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

[commit-bot: I haz the power, 2017-02-09 04:23:02]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bret, 2017-02-09 21:50:41]

The CQ bit was checked by bsep@chromium.org

[commit-bot: I haz the power, 2017-02-09 21:51:10]

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, 2017-02-09 22:59:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486677041454050,
"parent_rev": "d290cbee660c449245346abe4a9f2c7948df4c9e", "commit_rev":
"341a3f42f1319415886f11e12a3f1b84c8dd182d"}

[commit-bot: I haz the power, 2017-02-09 23:00:00]

Description was changed from

==========
Change CSSParserContext to have a Document handle (vs UseCounter).

This is a refactor only, no functionality should change. This gives
CSSParserContext a WeakMember reference to a Document instead of a raw
pointer to a UseCounter. Also gave CSSParserContext a count method so
clients aren't forced to check for isUseCounterRecordingEnabled every
time they want to count something. Also moved some getter methods around
because some of them weren't necessary any more.

BUG=668251
==========

to

==========
Change CSSParserContext to have a Document handle (vs UseCounter).

This is a refactor only, no functionality should change. This gives
CSSParserContext a WeakMember reference to a Document instead of a raw
pointer to a UseCounter. Also gave CSSParserContext a count method so
clients aren't forced to check for isUseCounterRecordingEnabled every
time they want to count something. Also moved some getter methods around
because some of them weren't necessary any more.

BUG=668251

Review-Url: https://codereview.chromium.org/2671173002
Cr-Commit-Position: refs/heads/master@{#449449}
Committed:
https://chromium.googlesource.com/chromium/src/+/341a3f42f1319415886f11e12a3f...
==========

[commit-bot: I haz the power, 2017-02-09 23:00:01]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/341a3f42f1319415886f11e12a3f...