1431855 - Flush notifications via the document in SVGDocumentWrapper.

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

See bug 1431852. Ideally nsIDocument::FlushPendingNotifications would be the only thing that calls over to the pres shell.

This callsite seems particularly easy to migrate.

I won't land this immediately, just to make it easier should any of the bugs depending on bug 1431852 cause any regression.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1431855: Flush notifications via the document in SVGDocumentWrapper.

Review commit: https://reviewboard.mozilla.org/r/214428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214428/

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8944104 [details]
Bug 1431855: Flush notifications via the document in SVGDocumentWrapper.

https://reviewboard.mozilla.org/r/214428/#review220090

::: image/SVGDocumentWrapper.cpp:422
(Diff revision 1)
>  }
>  
>  void
>  SVGDocumentWrapper::FlushLayout()
>  {
> -  nsCOMPtr<nsIPresShell> presShell;
> +  if (nsIDocument* doc = GetDocument()) {

I faintly recall that there might be a compiler warning that requires [under -Werror] that you put an extra layer of parens around assignments like this, as a hint that you really do want an assignment and this wasn't a typo'd equality-check...  Anyway, keep an eye out for that possibly causing an error in Try runs before you land this.

(I'm not sure it'd be triggered in this scenario when the variable is actually being *declared* inside of the condition... It's more of a possible-typo in cases where the variable was previously declared outside of the condition.)

Anyway -- I'd marinally prefer to pull out the assignment and just do "if (doc) {", to avoid the misreading-as-an-equality-check issue... but I don't feel strongly to tick "open an issue". :)  Up to you.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 8944104 [details]
> Bug 1431855: Flush notifications via the document in SVGDocumentWrapper.
> 
> https://reviewboard.mozilla.org/r/214428/#review220090
> 
> ::: image/SVGDocumentWrapper.cpp:422
> (Diff revision 1)
> >  }
> >  
> >  void
> >  SVGDocumentWrapper::FlushLayout()
> >  {
> > -  nsCOMPtr<nsIPresShell> presShell;
> > +  if (nsIDocument* doc = GetDocument()) {
> 
> I faintly recall that there might be a compiler warning that requires [under
> -Werror] that you put an extra layer of parens around assignments like this,
> as a hint that you really do want an assignment and this wasn't a typo'd
> equality-check...  Anyway, keep an eye out for that possibly causing an
> error in Try runs before you land this.
> 
> (I'm not sure it'd be triggered in this scenario when the variable is
> actually being *declared* inside of the condition... It's more of a
> possible-typo in cases where the variable was previously declared outside of
> the condition.)
> 
> Anyway -- I'd marinally prefer to pull out the assignment and just do "if
> (doc) {", to avoid the misreading-as-an-equality-check issue... but I don't
> feel strongly to tick "open an issue". :)  Up to you.

I'm fairly sure that this syntax is fine (because due to the declaration there's no possible typo, as you noted). I've used this for long and we have a fair amount of uses in nsCSSFrameConstructor (grep for "if (nsIFrame*"), for example.

I tend to like it a lot since it's a bit more compact and makes impossible to misuse the variable outside of the null-check, for example:

nsIDocument* doc = GetDocument();
if (doc) {
    doc->DoFoo();
}

// Tons of lines below...

doc->Bar(); // null-deref!

while with this pattern it'd be impossible.

Of course in this case it doesn't matter much. If you feel strongly about it I could also do something like:

nsIDocument* doc = GetDocument();
if (!doc) {
  return;
}

doc->FlushPendingNotifications(..);

Anyway, in this case it's not a big deal I think :)

Comment 4

[Daniel Holbert [:dholbert]]

Yeah, I agree that this syntax is better at scoping the variable to the if(...) block, which is a nice benefit.  If you've been using it elsewhere, then it sounds fine to use it here too. Proceed! :D

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3)
> I tend to like it a lot since it's a bit more compact and makes impossible
> to misuse the variable outside of the null-check, for example:

Yes, this.

FWIW declaration of a variable in an |if| condition is not only an increasingly common pattern in C++, but it's an extremely common pattern in Rust. We need to get used to it IMO. Also I just personally really like the lifetime being nice and clear and the code more compact. ;)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/375bc75671cc
Flush notifications via the document in SVGDocumentWrapper. r=dholbert

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/375bc75671cc