Closed Bug 1516007 Opened Last year Closed 11 months ago

Avoid extra work in nsIDocument::ScrollToRef

Categories

(Core :: DOM: Navigation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

Attachments

(2 files)

We can avoid making an extra copy when unescaping `mScrollToRef` by using
`NS_UnescapeURL` instead of `nsUnescape`. Additionally we can avoid calling
`GoToAnchor` a second time if nothing was unescaped.
Comment on attachment 9032999 [details] [diff] [review]
Avoid extra work in nsIDocument::ScrollToRef

Review of attachment 9032999 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +8821,5 @@
>  
>      if (NS_FAILED(rv)) {
> +      nsAutoCString buff;
> +      const nsACString& unescapedRef =
> +        NS_UnescapeURL(mScrollToRef, /*aFlags =*/ 0, buff);

I'm reasonably sure `aFlags == 0` is equivalent to nsUnescape, but it would be nice to have a second set of eyes on that.
Comment on attachment 9032999 [details] [diff] [review]
Avoid extra work in nsIDocument::ScrollToRef

Review of attachment 9032999 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +8821,5 @@
>  
>      if (NS_FAILED(rv)) {
> +      nsAutoCString buff;
> +      const nsACString& unescapedRef =
> +        NS_UnescapeURL(mScrollToRef, /*aFlags =*/ 0, buff);

Yeah, they look the same to me too...

@@ +8824,5 @@
> +      const nsACString& unescapedRef =
> +        NS_UnescapeURL(mScrollToRef, /*aFlags =*/ 0, buff);
> +
> +      // This attempt is only necessary if characters were unescaped.
> +      if (&unescapedRef == &buff) {

Why use this variant of the function so that you have to do this gymnastics?  If you use this version that just returns a bool, your life would be a lot simpler: https://searchfox.org/mozilla-central/rev/70a4778dd84ec94b4b3a2537fd28482de45b9a00/xpcom/io/nsEscape.h#145
Attachment #9032999 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari from comment #3)
> Comment on attachment 9032999 [details] [diff] [review]
> Avoid extra work in nsIDocument::ScrollToRef
> 
> Review of attachment 9032999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDocument.cpp
> @@ +8821,5 @@
> >  
> >      if (NS_FAILED(rv)) {
> > +      nsAutoCString buff;
> > +      const nsACString& unescapedRef =
> > +        NS_UnescapeURL(mScrollToRef, /*aFlags =*/ 0, buff);
> 
> Yeah, they look the same to me too...
> 
> @@ +8824,5 @@
> > +      const nsACString& unescapedRef =
> > +        NS_UnescapeURL(mScrollToRef, /*aFlags =*/ 0, buff);
> > +
> > +      // This attempt is only necessary if characters were unescaped.
> > +      if (&unescapedRef == &buff) {
> 
> Why use this variant of the function so that you have to do this gymnastics?
> If you use this version that just returns a bool, your life would be a lot
> simpler:
> https://searchfox.org/mozilla-central/rev/
> 70a4778dd84ec94b4b3a2537fd28482de45b9a00/xpcom/io/nsEscape.h#145

I guess I was trying to avoid blame below, but I agree that's cleaner. I'll switch before landing.
Priority: -- → P3
This is the modified version that you suggested. It's a bit uglier:
 - I had to unpack the string in order to call this version
 - Needed a tri-state later checking if the buffer was used

Let me know if you still prefer it.
Attachment #9034492 - Flags: review?(ehsan)
Comment on attachment 9034492 [details] [diff] [review]
Avoid extra work in nsIDocument::ScrollToRef

Review of attachment 9034492 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  I do prefer this version over relying on the address of the returned value tbh.  Even though neither is clearly superior to the other, I must say...
Attachment #9034492 - Flags: review?(ehsan) → review+
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/632ce013755e
Avoid extra work in nsIDocument::ScrollToRef. r=ehsan
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.