Closed Bug 1422197 Opened 7 years ago Closed 7 years ago

Add fast path to get DocGroup in binding code for [CEReactions]

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(1 file)

Bug 1422197 - Add fast path to get DocGroup in binding code for [CEReactions];
59 bytes, text/x-review-board-request
smaug
: review+
Details 
(In reply to Edgar Chen [:edgar] from comment #1)
> Created attachment 8934074 [details]
> Bug 1422197 - Add fast path to get DocGroup in binding code for
> [CEReactions];
> 
> Review commit: https://reviewboard.mozilla.org/r/205022/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/205022/

Test: https://bugzilla.mozilla.org/attachment.cgi?id=8932732

Base rev: 3f5d48c08903

Without patch:
 - pref off: average time is about 101 ms
 - pref on:  average time is about 122 ms

With patch:
 - pref off: average time is about 100 ms
 - pref on:  average time is about 114 ms
Comment on attachment 8934074 [details]
Bug 1422197 - Add fast path to get DocGroup in binding code for [CEReactions];

This is not the final patch, this patch doesn't make example implementation generation work. But I would like to have a early feedback to see if I am in a right direction.

Basically, if a WebIDL interface contains an operation or attribute that annotated with [CEReactions], the relevant classes need to provide an GetDocGroup() method that returns the associated DocGroup.
Attachment #8934074 - Flags: feedback?(bugs)
Comment on attachment 8934074 [details]
Bug 1422197 - Add fast path to get DocGroup in binding code for [CEReactions];

Looks reasonable. Need to ensure all those mFoo->OwnerDoc() are safe, in other words that mFoo isn't ever null.
Attachment #8934074 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #4)
> Need to ensure all those mFoo->OwnerDoc() are safe, in other words that mFoo isn't ever null.

Will do, thank you!!
And with that I mean that either add a null check or explain why mFoo can't be null.
(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #6)
> And with that I mean that either add a null check or explain why mFoo can't
> be null.

Roger that!
(In reply to Edgar Chen [:edgar] from comment #8)
> Comment on attachment 8934074 [details]
> Bug 1422197 - Add fast path to get DocGroup in binding code for
> [CEReactions];
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/205022/diff/1-2/

Changes of this version
- Add support for example generator to generate relevant code.
- Add support for js implemented interface to generate relevant code.

I am still working on ensuring all mFoo->OwnerDoc() are safe (add null check or explanation).
Attachment #8934074 - Flags: review?(bugs)
Comment on attachment 8934074 [details]
Bug 1422197 - Add fast path to get DocGroup in binding code for [CEReactions];

https://reviewboard.mozilla.org/r/205022/#review213336

::: layout/style/nsCSSRules.cpp:920
(Diff revision 3)
> +  if (!mRule) {
> +    return nullptr;
> +  }
> +
> +  nsIDocument* document = mRule->GetDocument();
> +  return mRule ? document->GetDocGroup() : nullptr;

You already null check mRule above.

But 
Is it guaranteed that GetDocument doesn't ever return null? If so, it should be renamed to Document(). Want to file a followup bug?

Or if not, null check the return value of GetDocument()

Or perhaps you meant to write
return document ? document->GetDocGroup() : nullptr

::: layout/style/nsCSSRules.cpp:1358
(Diff revision 3)
>  }
>  
> +DocGroup*
> +nsCSSPageStyleDeclaration::GetDocGroup() const
> +{
> +  return mRule ? mRule->GetDocument()->GetDocGroup() : nullptr;

Is it guaranteed that GetDocument doesn't ever return null? If so, it should be renamed to Document(). Want to file a followup bug?

Or if not, null check the return value of GetDocument()
Attachment #8934074 - Flags: review?(bugs) → review+
Comment on attachment 8934074 [details]
Bug 1422197 - Add fast path to get DocGroup in binding code for [CEReactions];

https://reviewboard.mozilla.org/r/205022/#review213336

> You already null check mRule above.
> 
> But 
> Is it guaranteed that GetDocument doesn't ever return null? If so, it should be renamed to Document(). Want to file a followup bug?
> 
> Or if not, null check the return value of GetDocument()
> 
> Or perhaps you meant to write
> return document ? document->GetDocGroup() : nullptr

I meant to write `return document ? document->GetDocGroup() : nullptr`.
(It seems like I still have jet lag :()

> Is it guaranteed that GetDocument doesn't ever return null? If so, it should be renamed to Document(). Want to file a followup bug?
> 
> Or if not, null check the return value of GetDocument()

GetDocument() could return null, I meant to write,
`
if (mRule) {
  return nullprt;
}

nsIDocument* document = mRule->GetDocument();
return document ? document->GetDocGroup() : nullptr;
`

Thank you for catching this.
Pushed by echen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57a108d1c90a
Add fast path to get DocGroup in binding code for [CEReactions]; r=smaug
https://hg.mozilla.org/mozilla-central/rev/57a108d1c90a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: