1447012 - Use waitForNewTab instead of progress listener in addJsonViewTab

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement)

Description

[Oriol Brufau [:Oriol]]

addJsonViewTab uses a progress listener which is a bit ugly. BrowserTestUtils.waitForNewTab does basically the same thing but the code becomes clearer.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Bug 1447012 - Use waitForNewTab instead of progress listener in addJsonViewTab

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

Comment 2

[Oriol Brufau [:Oriol]]

Comment on attachment 8960183 [details]
Bug 1447012 - Use waitForNewTab instead of progress listener in addJsonViewTab

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

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: server-error.png

@Oriol, can you please update the patch I can't publish it from some reason. I am seeing HTTP 500 INTERNAL SERVER ERROR any time I press the review "Publish" button.

Otherwise, the patch looks good.

Honza

Comment 4

[Oriol Brufau [:Oriol]]

Attachment: Bug 1447012 - Use waitForNewTab instead of progress listener in addJsonViewTab

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

Comment 5

[Oriol Brufau [:Oriol]]

I also thought it was strange that two mozreview-request comments appeared, and in rb the title of the patch was not shown.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8960226 [details]
Bug 1447012 - Use waitForNewTab instead of progress listener in addJsonViewTab

https://reviewboard.mozilla.org/r/228988/#review234706

Thanks for the patch!
R+ assuming try is green
Honza

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41097954197f
Use waitForNewTab instead of progress listener in addJsonViewTab r=Honza

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/41097954197f

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure I understand this change. Now we're racing two separate tab loads against each other and using whichever one happens to return back to us first? Why do we do that?

Comment 10

[Oriol Brufau [:Oriol]]

If I remember correctly, the test browser_jsonview_content_type.js uses some invalid content types. Some of them just trigger a download instead of loading anything, and then one of these promises didn't resolve. And the other promise also had some problems in other cases, like resolving too late or something. So I decided to use them both.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, then this code should probably document exactly what it's doing and why, since it's doing two very different loads with different conditions on resolving and then just using whichever of them resolves first, as if it did not matter what the conditions are, etc...

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Oriol: can you please file a follow up and provide detailed comment that explains the logic, thanks!

Honza

Comment 13

[Oriol Brufau [:Oriol]]

Filed bug 1603962