1428342 - Make Places queries directly inherit options from their parent

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Marco Bonardo [:mak]]

This simplifies some of the results code and helps with virtualization of the left pane folder.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1428342 - Make Places queries directly inherit options from their parent.

Rather then trying to guess options from the parent or the root node, make query nodes directly
inherit some options from their parent.

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

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

https://reviewboard.mozilla.org/r/211408/#review217446

Looks good, though there's some test failures on try that need looking at.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

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

Comment 4

[Marco Bonardo [:mak]]

The patch was breaking the Downloads view in the Library, because some code was doing apparently pointless changes...
In any case, I'd delay landing any virtualization work (including this) to 60 at this point, to stay on the safe side. In the end this patch is mostly necessary for the work in bug 1423896 that is unlikely to land during the soft freeze.

Comment 5

[Marco Bonardo [:mak]]

hm, some other failures on Try, different ones :\

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

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

Comment 7

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f7f3b0de8a82ce4d6048c3dc14c1bd51802fd9f

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

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

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

https://reviewboard.mozilla.org/r/211408/#review217844

::: toolkit/components/places/nsNavHistoryResult.h:477
(Diff revision 4)
>    // this is set to the default in the constructor.
>    bool mExpanded;
>  
>    // Filled in by the result type generator in nsNavHistory.
>    nsCOMArray<nsNavHistoryResultNode> mChildren;
> -
> +  nsCOMPtr<nsNavHistoryQueryOptions> mOriginalOptions;

I think it would be nice to add some documentation around mOriginalOptions and mOptions as to why they are needed, so we can have it at a glance in future.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

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

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

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

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8941137 [details]
Bug 1428342 - Make Places queries directly inherit options from their parent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/211408/diff/6-7/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cba9a29be139
Make Places queries directly inherit options from their parent. r=standard8

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/cba9a29be139