1376929 - Fix places-related mochitests in chrome/ for async-transactions

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P1)

Description

[Mark Banner (:standard8)]

There are various tests in chrome/ that fail when we try to turn on async-transactions.

We should fix those.

Comment 1

[Marco Bonardo [:mak]]

stealing bug.

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1376929 - Fix places-related mochitests in chrome/ for async-transactions.

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

Comment 3

[Marco Bonardo [:mak]]

Looks like there are some failures to look into.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8882723 [details]
Bug 1376929 - Fix places-related mochitests in chrome/ for async-transactions.

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

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8882723 [details]
Bug 1376929 - Fix places-related mochitests in chrome/ for async-transactions.

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

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8882723 [details]
Bug 1376929 - Fix places-related mochitests in chrome/ for async-transactions.

https://reviewboard.mozilla.org/r/153816/#review159302

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f5e741ab2b6
Fix places-related mochitests in chrome/ for async-transactions. r=standard8

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1f5e741ab2b6

Comment 9

[Dmitry Gutov]

> -    if (!aURI || !aTags || !Array.isArray(aTags)) {
> -      throw Cr.NS_ERROR_INVALID_ARG;
> +    if (!aURI || !aTags || !Array.isArray(aTags) || aTags.length == 0) {
> +      throw Components.Exception("Invalid value for tags", Cr.NS_ERROR_INVALID_ARG);

Why did you do that? An empty array is still an array. getTagsForURI can return an empty array; why am I not allowed to pass the returned value straight to tagURI?

This broke https://addons.mozilla.org/en-US/firefox/addon/bmreplace/.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Dmitry Gutov from comment #9)
> Why did you do that? An empty array is still an array. getTagsForURI can
> return an empty array; why am I not allowed to pass the returned value
> straight to tagURI?

because trying to tag with an empty array may hide bugs in the code. Suppose some code is trying to add tags, but due to a bug it ends up passing an empty array every time.

Comment 11

[Dmitry Gutov]

> Suppose some code is trying to add tags, but due to a bug it ends up passing an empty array every time.

That is silly. An imaginary bug that ends up generating an empty array unintentionally could just as well generate a non-empty array with undesired values.

This is a backward incompatible change. Please revert it.

Comment 12

[Marco Bonardo [:mak]]

why? the extension can be fixed easily with a simple if, in a few minutes. I prefer having the API helping us finding unexpected behaviors in the UI code.

Comment 13

[Dmitry Gutov]

I doubt this is the only extension that will need to be fixed. And debugging this breakage took more than a few minutes.