Make CSSParserContext be garbage collected.

Make CSSParserContext be garbage collected.

This is a refactor only, no functionality should change. This allocates
CSSParserContext on the heap and wires it up to be garbage collected.
This is necessary so in the future it can have a handle to a Document
(also a GC'ed class) instead of a UseCounter.

Also includes some minor API changes to CSSParserContext that I think
make it more readable, since I'm changing every callsite of the class
already anyway.

TBR=haraken@chromium.org
BUG=668251
Review-Url: https://codereview.chromium.org/2616093003
Cr-Commit-Position: refs/heads/master@{#443792}
Committed: https://chromium.googlesource.com/chromium/src/+/37b65afaf587c59143a6dca1ea75a505d6c696c6

[Bret, 2017-01-07 01:44:23]

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

[commit-bot: I haz the power, 2017-01-07 01:44:36]

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-01-07 01:47:11]

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

[commit-bot: I haz the power, 2017-01-07 01:47:12]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Bret, 2017-01-07 01:57:56]

Patchset #4 (id:60001) has been deleted

[Bret, 2017-01-07 02:02:11]

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

[commit-bot: I haz the power, 2017-01-07 02:02:24]

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-01-07 02:14:02]

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

[commit-bot: I haz the power, 2017-01-07 02:14:03]

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...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bret, 2017-01-10 23:34:30]

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

[commit-bot: I haz the power, 2017-01-10 23:35:14]

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-01-11 00:16:03]

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

[commit-bot: I haz the power, 2017-01-11 00:16:04]

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

[Bret, 2017-01-11 00:35:20]

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

[commit-bot: I haz the power, 2017-01-11 00:36:15]

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

[Bret, 2017-01-11 00:51:30]

Description was changed from

==========
Make CSSParserContext be garbage collected.

This is a refactor only, no functionality should change. This allocates
CSSParserContext on the heap and wires it up to be garbage collected.
This is necessary so in the future it can have a handle to a Document
instead of a UseCounter.

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

to

==========
Make CSSParserContext be garbage collected.

This is a refactor only, no functionality should change. This allocates
CSSParserContext on the heap and wires it up to be garbage collected.
This is necessary so in the future it can have a handle to a Document
(also a GC'ed class) instead of a UseCounter.

Also includes some minor API changes to CSSParserContext that I think
make it more readable, since I'm changing every callsite of the class
already anyway.

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

[Bret, 2017-01-11 01:22:46]

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

[commit-bot: I haz the power, 2017-01-11 01:23:38]

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

[Bret, 2017-01-11 02:21:01]

bsep@chromium.org changed reviewers:
+ rune@opera.com

[commit-bot: I haz the power, 2017-01-11 02:35:45]

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

[commit-bot: I haz the power, 2017-01-11 02:35:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[haraken, 2017-01-11 02:42:17]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[haraken, 2017-01-11 02:42:18]

+oilpan-reviews

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSGroupingRule.cpp (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSGroupingRule.cpp:66: new
CSSParserContext(parserContext(), UseCounter::getFrom(styleSheet));

Maybe we want to have a factory method. CSSParserContext::create(...).

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:120:
Persistent<const CSSParserContext> m_context;

This object is stack-allocated, so you can just use a raw pointer.

keishi@: Can we add a check to the clang plugin to detect Persistent handles in
stack-allocated objects?

[rune, 2017-01-11 12:59:26]

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSGroupingRule.cpp (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSGroupingRule.cpp:66: new
CSSParserContext(parserContext(), UseCounter::getFrom(styleSheet));
On 2017/01/11 02:42:18, haraken wrote:
> 
> Maybe we want to have a factory method. CSSParserContext::create(...).

+1

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp:79: static
CSSParserContext* strictContext =
Doesn't it need to be Persistent?

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResource.cpp
(right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResource.cpp:202: if
(*(m_parsedStyleSheetCache->parserContext()) != *context)
I don't think you need parentheses here. *a->b() is the same as *(a->b()), not
(*a)->b().

[haraken, 2017-01-11 13:04:54]

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp:79: static
CSSParserContext* strictContext =
On 2017/01/11 12:59:26, rune wrote:
> Doesn't it need to be Persistent?

Nice catch! This needs to be DEFINE_STATIC_LOCAL(Persistent<>, ...)

[sof, 2017-01-11 13:09:21]

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp:79: static
CSSParserContext* strictContext =
On 2017/01/11 13:04:54, haraken wrote:
> On 2017/01/11 12:59:26, rune wrote:
> > Doesn't it need to be Persistent?
> 
> Nice catch! This needs to be DEFINE_STATIC_LOCAL(Persistent<>, ...)

DEFINE_STATIC_LOCAL(T, ...) will implicitly wrap a Persistent<> around the T if
it is a GCed type.

[Bret, 2017-01-11 23:05:15]

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSGroupingRule.cpp (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSGroupingRule.cpp:66: new
CSSParserContext(parserContext(), UseCounter::getFrom(styleSheet));
On 2017/01/11 12:59:26, rune wrote:
> On 2017/01/11 02:42:18, haraken wrote:
> > 
> > Maybe we want to have a factory method. CSSParserContext::create(...).
> 
> +1

Added for CSSStyleSheet and StyleSheetContents. Though I wonder if it should be
named CSSParserContext::copy instead?

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp:79: static
CSSParserContext* strictContext =
On 2017/01/11 13:09:21, sof wrote:
> On 2017/01/11 13:04:54, haraken wrote:
> > On 2017/01/11 12:59:26, rune wrote:
> > > Doesn't it need to be Persistent?
> > 
> > Nice catch! This needs to be DEFINE_STATIC_LOCAL(Persistent<>, ...)
> 
> DEFINE_STATIC_LOCAL(T, ...) will implicitly wrap a Persistent<> around the T
if
> it is a GCed type.

Done... though needing "&strictContext" doesn't seem right to me, but it won't
compile otherwise. Is this correct?

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:120:
Persistent<const CSSParserContext> m_context;
On 2017/01/11 02:42:18, haraken wrote:
> 
> This object is stack-allocated, so you can just use a raw pointer.
> 
> keishi@: Can we add a check to the clang plugin to detect Persistent handles
in
> stack-allocated objects?

It does not compile with a raw pointer. Clang gives me "[blink-gc] Raw pointer
field 'm_context' to a GC managed class"

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResource.cpp
(right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResource.cpp:202: if
(*(m_parsedStyleSheetCache->parserContext()) != *context)
On 2017/01/11 12:59:26, rune wrote:
> I don't think you need parentheses here. *a->b() is the same as *(a->b()), not
> (*a)->b().

Done.

[haraken, 2017-01-12 04:51:14]

LGTM with comments.

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:120:
Persistent<const CSSParserContext> m_context;
On 2017/01/11 23:05:15, Bret Sepulveda wrote:
> On 2017/01/11 02:42:18, haraken wrote:
> > 
> > This object is stack-allocated, so you can just use a raw pointer.
> > 
> > keishi@: Can we add a check to the clang plugin to detect Persistent handles
> in
> > stack-allocated objects?
> 
> It does not compile with a raw pointer. Clang gives me "[blink-gc] Raw pointer
> field 'm_context' to a GC managed class"

Sorry, it should be Member<const CSSParserContext>.

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ActiveStyleSheetsTest.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ActiveStyleSheetsTest.cpp:26:
StyleSheetContents::create(new CSSParserContext(HTMLStandardMode));

Blink prefers a factory pattern. Can you replace all the new statements with
CSSParserContext::create()?

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFace.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFace.cpp:70:
context->setUseCounter(UseCounter::getFrom(document));

It looks not really nice that we have to create CSSParserContext and call
setUserCounter separately.

Once your refactoring is done, what will it look like in the end?

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParser.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParser.cpp:84:
static_cast<StyleSheetContents*>(nullptr));

What is this change for?

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp:96: return
&strictContext;

Or you can change the type of the return value to CSSParserContext&, since the
return value is guaranteed to be not null.

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.h (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.h:32:
CSSParserContext(CSSParserMode, SelectorProfile = DynamicProfile);

Put these constructors in a private section.

[Bret, 2017-01-13 02:15:28]

Changed the interface of CSSParserContext a bit to accommodate the factory
functions. PTAL

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h (right):

https://codereview.chromium.org/2616093003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.h:120:
Persistent<const CSSParserContext> m_context;
On 2017/01/12 04:51:13, haraken wrote:
> On 2017/01/11 23:05:15, Bret Sepulveda wrote:
> > On 2017/01/11 02:42:18, haraken wrote:
> > > 
> > > This object is stack-allocated, so you can just use a raw pointer.
> > > 
> > > keishi@: Can we add a check to the clang plugin to detect Persistent
handles
> > in
> > > stack-allocated objects?
> > 
> > It does not compile with a raw pointer. Clang gives me "[blink-gc] Raw
pointer
> > field 'm_context' to a GC managed class"
> 
> Sorry, it should be Member<const CSSParserContext>.

Done.

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ActiveStyleSheetsTest.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ActiveStyleSheetsTest.cpp:26:
StyleSheetContents::create(new CSSParserContext(HTMLStandardMode));
On 2017/01/12 04:51:13, haraken wrote:
> 
> Blink prefers a factory pattern. Can you replace all the new statements with
> CSSParserContext::create()?

Done.

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/FontFace.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/FontFace.cpp:70:
context->setUseCounter(UseCounter::getFrom(document));
On 2017/01/12 04:51:13, haraken wrote:
> 
> It looks not really nice that we have to create CSSParserContext and call
> setUserCounter separately.
> 
> Once your refactoring is done, what will it look like in the end?

Yes, I'm wasn't entirely happy with this. I changed around the construction
interface when I made all the factories so now it doesn't need to be a separate
statement.

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParser.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParser.cpp:84:
static_cast<StyleSheetContents*>(nullptr));
On 2017/01/12 04:51:13, haraken wrote:
> 
> What is this change for?

There were some calls that were trying to pass in 0/nullptr for
StyleSheetContents*, but since I made the version that takes a CSSParserContext
into a pointer it became ambiguous. I decided to add this overload to handle it,
but if you think it's too much I can move the
static_cast<StyleSheetContents*>(nullptr) to the callsites instead.

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.cpp:96: return
&strictContext;
On 2017/01/12 04:51:13, haraken wrote:
> 
> Or you can change the type of the return value to CSSParserContext&, since the
> return value is guaranteed to be not null.

I think this is okay, as long as it isn't wrong. This way CSSParserContext is a
pointer everywhere.

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserContext.h (right):

https://codereview.chromium.org/2616093003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserContext.h:32:
CSSParserContext(CSSParserMode, SelectorProfile = DynamicProfile);
On 2017/01/12 04:51:13, haraken wrote:
> 
> Put these constructors in a private section.

Done.

[haraken, 2017-01-13 02:19:11]

LGTM

[Bret, 2017-01-14 01:13:11]

Description was changed from

==========
Make CSSParserContext be garbage collected.

This is a refactor only, no functionality should change. This allocates
CSSParserContext on the heap and wires it up to be garbage collected.
This is necessary so in the future it can have a handle to a Document
(also a GC'ed class) instead of a UseCounter.

Also includes some minor API changes to CSSParserContext that I think
make it more readable, since I'm changing every callsite of the class
already anyway.

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

to

==========
Make CSSParserContext be garbage collected.

This is a refactor only, no functionality should change. This allocates
CSSParserContext on the heap and wires it up to be garbage collected.
This is necessary so in the future it can have a handle to a Document
(also a GC'ed class) instead of a UseCounter.

Also includes some minor API changes to CSSParserContext that I think
make it more readable, since I'm changing every callsite of the class
already anyway.

TBR=haraken@chromium.org
BUG=668251
==========

[Bret, 2017-01-14 01:13:31]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-01-14 01:13:32]

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/2616093003/#ps200001
(title: "fixed merge")

[commit-bot: I haz the power, 2017-01-14 01:14:00]

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-01-14 01:58:50]

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

[commit-bot: I haz the power, 2017-01-14 01:58:53]

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_...)

[Bret, 2017-01-14 02:35:31]

The CQ bit was checked by bsep@chromium.org

[Bret, 2017-01-14 02:35:32]

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/2616093003/#ps220001
(title: "fix fuzzer compile again")

[commit-bot: I haz the power, 2017-01-14 02:35:51]

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-01-14 06:10:36]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484361331471570,
"parent_rev": "a74a47aec12401d3ea3643aea1d8b66d908c32cc", "commit_rev":
"37b65afaf587c59143a6dca1ea75a505d6c696c6"}

[commit-bot: I haz the power, 2017-01-14 06:11:53]

Description was changed from

==========
Make CSSParserContext be garbage collected.

This is a refactor only, no functionality should change. This allocates
CSSParserContext on the heap and wires it up to be garbage collected.
This is necessary so in the future it can have a handle to a Document
(also a GC'ed class) instead of a UseCounter.

Also includes some minor API changes to CSSParserContext that I think
make it more readable, since I'm changing every callsite of the class
already anyway.

TBR=haraken@chromium.org
BUG=668251
==========

to

==========
Make CSSParserContext be garbage collected.

This is a refactor only, no functionality should change. This allocates
CSSParserContext on the heap and wires it up to be garbage collected.
This is necessary so in the future it can have a handle to a Document
(also a GC'ed class) instead of a UseCounter.

Also includes some minor API changes to CSSParserContext that I think
make it more readable, since I'm changing every callsite of the class
already anyway.

TBR=haraken@chromium.org
BUG=668251

Review-Url: https://codereview.chromium.org/2616093003
Cr-Commit-Position: refs/heads/master@{#443792}
Committed:
https://chromium.googlesource.com/chromium/src/+/37b65afaf587c59143a6dca1ea75...
==========

[commit-bot: I haz the power, 2017-01-14 06:11:55]

Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/37b65afaf587c59143a6dca1ea75...

[esprehn, 2017-03-22 20:20:38]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2017-03-22 20:20:39]

This means we're allocating thousands of garbage collected objects when setting
inline style properties or calling querySelector in a typical page. This patch
also switched all the call sites to use a ptr instead of a reference, but the
reference signaled that the value could never be null on purpose. We should
switch those back.

I'd like to better understand why this should be garbage collected? It's a state
object we only create when parsing CSS values, the lifetime is well known.

[dcheng, 2017-03-22 20:29:28]

On 2017/03/22 20:20:39, esprehn wrote:
> This means we're allocating thousands of garbage collected objects when
setting
> inline style properties or calling querySelector in a typical page. This patch
> also switched all the call sites to use a ptr instead of a reference, but the
> reference signaled that the value could never be null on purpose. We should
> switch those back.
> 
> I'd like to better understand why this should be garbage collected? It's a
state
> object we only create when parsing CSS values, the lifetime is well known.

I think the motivation was described in the CL description (to hold a handle to
Document), though I'm curious why it couldn't simply be a Persistent reference.

[Bret, 2017-03-22 20:59:51]

On 2017/03/22 20:29:28, dcheng wrote:
> On 2017/03/22 20:20:39, esprehn wrote:
> > This means we're allocating thousands of garbage collected objects when
> setting
> > inline style properties or calling querySelector in a typical page. This
patch
> > also switched all the call sites to use a ptr instead of a reference, but
the
> > reference signaled that the value could never be null on purpose. We should
> > switch those back.
> > 
> > I'd like to better understand why this should be garbage collected? It's a
> state
> > object we only create when parsing CSS values, the lifetime is well known.
> 
> I think the motivation was described in the CL description (to hold a handle
to
> Document), though I'm curious why it couldn't simply be a Persistent
reference.

I originally tried to make it a Persistent reference but I couldn't make it
work. CSSParserContext was a member of a couple other GC'ed classes
(CSSLazyParsingState and StyleSheetContents) which can't have Persistent
references. And since CSSParserContext wasn't GC'ed I couldn't make it a Member
reference. I found some advice that basically said "in this case you just have
to make it a GC'ed class" so that's what I did.

I think there is further refactoring possible here. I turned a lot of implicit
copies of CSSParserContext into explicit copies because they were switching
between UseCounter recording and non-recording mode. That seems odd to me, since
naively I'd think we would almost always want UseCounter recording on, but I
don't know the considerations well enough to make that call.

Also I turned the references into pointers because it seemed like the style was
to use pointers for GC'ed classes, since they have to be pointers at their
declaration site.

[esprehn, 2017-03-22 22:11:19]

On 2017/03/22 at 20:59:51, bsep wrote:
> ...
> 
> Also I turned the references into pointers because it seemed like the style
was to use pointers for GC'ed classes, since they have to be pointers at their
declaration site.

We use references for GC types all the time, it signals that the type is not
nullable.

[haraken, 2017-03-22 23:38:38]

If CSSParserContext is allocated so much, yes, we need to rethink about moving
it off Oilpan's heap.