1280876 - Find toolbar influences New Tab page

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Anne (:annevk)]

Attachment: new tab screen shot.png

It seems if you did an inline search, that can influence the thumbnails for the New Tab page.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]: Regression

Comment 2

[Marcia Knous [:marcia]]

Feature has been backed out so not tracking these bugs for 50.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1280876 - don't take screenshots of pages that have the findbar open.

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

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

https://reviewboard.mozilla.org/r/67776/#review65108

::: toolkit/components/thumbnails/PageThumbUtils.jsm:284
(Diff revision 1)
> +    // Don't take screenshots when the findbar is active.
> +    if (aDocument.mozFinderHighlightVisible) {
> +      return false;
> +    }

Can you just query if the findbar is open? 

Something not as ugly as `gBrowser.getBrowserForDocument(aDocument)._remoteFinder` and something that works for e10s and non-e10s, but similar?

::: toolkit/modules/FinderHighlighter.jsm:234
(Diff revision 1)
>  
>      window = window || this.finder._getWindow();
>      let anonNode = this._maybeCreateModalHighlightNodes(window);
>      this._addModalHighlightListeners(window);
>  
> +    window.document.mozFinderHighlightVisible = true;

I don't particularly like us adding properties to the document. Will they be visible to content? I'm assuming so, since the overlay needed to be added to anonymous content.

::: toolkit/modules/FinderHighlighter.jsm:265
(Diff revision 1)
>            this._unhookListenersAtIndex(x);
>          }
>        }
>      }
>  
> +    delete window.document.mozFinderHighlightVisible;

window.document.mozFinderHighlightVisible = false;

Comment 5

[Mike de Boer [:mikedeboer]]

Thanks for the review!

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Can you just query if the findbar is open? 

Aah, yes... why didn't I think of that?

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Aah, yes... why didn't I think of that?

Apparently I did, but is complicated due to the fact that the findbar is in the chrome process and the highlighter in the content process. I need to think of something else here.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67776/diff/1-2/

Comment 8

[Mike de Boer [:mikedeboer]]

https://reviewboard.mozilla.org/r/67776/#review65340

::: docshell/base/nsDocShell.cpp:819
(Diff revision 2)
>    , mFrameType(FRAME_TYPE_REGULAR)
>    , mPrivateBrowsingId(0)
>    , mParentCharsetSource(0)
>    , mJSRunToCompletionDepth(0)
>    , mTouchEventsOverride(nsIDocShell::TOUCHEVENTS_OVERRIDE_NONE)
> +  , mFindbarOpen(false)

Compiler gives a warning here about the order of initialization, but I don't think that matters much in this case...

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Compiler gives a warning here about the order of initialization, but I don't think that matters much in this case...

It matters, both in terms of efficiency and compiler warning spew, and you should fix the warning by ordering your initializers the same as you order your members.

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/67776/#review65354

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67776/diff/2-3/

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> It matters, both in terms of efficiency and compiler warning spew, and you
> should fix the warning by ordering your initializers the same as you order
> your members.

Aha! OK, thanks for the explanation. It's part of the basics, I know, but I learn everyday...

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

https://reviewboard.mozilla.org/r/67776/#review65378

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

https://reviewboard.mozilla.org/r/67776/#review67506

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

https://reviewboard.mozilla.org/r/67776/#review67508

Oh, looks like mozreview UI now disappears headers if you don't explicitly save them...

What I just _tried_ to say, and what I said a week ago when mozreview _also_ disappeared my comments, is that it's not clear to me why this state needs to live on docshell as opposed to the browser or tabbrowser or some other part of the frontend that the findbar is coordinating with anyway.  Is this state going to be relevant to subframes, for example?  I don't think so....

Comment 16

[Boris Zbarsky [:bzbarsky]]

I filed bug 1293565 on the mozreview UI bustage....

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> What I just _tried_ to say, and what I said a week ago when mozreview _also_
> disappeared my comments, is that it's not clear to me why this state needs
> to live on docshell as opposed to the browser or tabbrowser or some other
> part of the frontend that the findbar is coordinating with anyway.  Is this
> state going to be relevant to subframes, for example?  I don't think so....

Everything but the findbar XUL (and the RemoteFinder.jsm message proxy) live in the content process(es) and there's no `gFindBar` global that can be referenced there to extract state from it.
Since the background thumbnailer (the change in PageThumbUtils.jsm is what makes this all work) runs in the content process too, and checks if it should store a content thumbnail synchronously, so adding an async message passing flow there would be a bit problematic.
At first I thought about using a JS expando prop on the document object - `mozFindbarOpen = true` whilst the findbar is open - but that'd be visible to the web.
Do you see another option? Thanks for checking the bug again!

Comment 18

[Boris Zbarsky [:bzbarsky]]

> but that'd be visible to the web

Er... no, it wouldn't.  The expando would be defined on the Xray; chrome would see it, but web content would not.

A bigger problem for the expando-on-the-document approach is that it would not persist across navigations, but the "findbar is open" state does.

Is there really no chrome object that corresponds to the tab but lives content-side??

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> Is there really no chrome object that corresponds to the tab but lives
> content-side??

No, but I can make a new content global... That's probably the best option, thanks for asking.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67776/diff/3-4/

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

https://reviewboard.mozilla.org/r/67776/#review67792

r=me with my nits fixed. Thanks mikedeboer!

::: toolkit/modules/BrowserUtils.jsm:315
(Diff revision 4)
> +   * @param  {nsIDocShell} docShell The docShell instance that a toolbar should
> +   *                                be interacting with
> +   * @param  {String}      [which]  Identifier of a specific toolbar
> +   * @return {Boolean}
> +   */
> +  isToolbarVisible(docShell, which = null) {

Out of curiosity, why are we defaulting this to null?

It looks like the null case means "there are no toolbars visible", but is that useful, especially given that we return false if we've never seen the window before?

It might be safer to just return toolbars.has(which), unless there's a good reason to care about the "we saw it once, but nothing is currently visible" case.

::: toolkit/modules/BrowserUtils.jsm:334
(Diff revision 4)
> +   * @param  {Boolean}     [visible] Whether the toolbar is visible. Optional,
> +   *                                 defaults to `true`.
> +   */
> +  trackToolbarVisibility(docShell, which, visible = true) {
> +    if (!this._visibleToolbarsMap)
> +      this._visibleToolbarsMap = new WeakMap();

I think we can probably just define \_visibleToolbarsMap directly on the BrowserUtils object safely, and be reasonably sure it exists.

Comment 22

[Mike de Boer [:mikedeboer]]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #21)
> Out of curiosity, why are we defaulting this to null?
> 
> It looks like the null case means "there are no toolbars visible", but is
> that useful, especially given that we return false if we've never seen the
> window before?
> 
> It might be safer to just return toolbars.has(which), unless there's a good
> reason to care about the "we saw it once, but nothing is currently visible"
> case.

Agreed.

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8775609 [details]
Bug 1280876 - don't take screenshots of pages that have the findbar open.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67776/diff/4-5/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ec1ef348547
don't take screenshots of pages that have the findbar open. r=jaws,mconley

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ec1ef348547

Comment 26

[Simona Badau, Desktop QA]

I couldn't reproduce the initial reported issue and can't reproduce it either on the latest Nightly 52.0a1 (Build ID: 20160928030201) - Win10, Mac OS X 10.11 and Ubuntu 16.04, therefore I can't verify it as fixed. 

Anne, can you please confirm that this issue is indeed fixed?

Comment 27

[Anne (:annevk)]

I haven't seen this recently. The only thing I notice is that search is still super slow and janky (and has some weird painting issues).

Comment 28

[Simona Badau, Desktop QA]

Based on Comment 27, setting the status of this issue to Verified Fixed. Please note that the new Find Toolbar feature is disabled by default on Firefox 51.