1402399 - Cannot clear titlePreface by calling browser.windows.update with empty string as titlePreface

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Rob Wu [:robwu]]

browser.windows.update(-2, {titlePreface: 'xxx'}); // sets title preface to 'xxx'
browser.windows.update(-2, {titlePreface: ''});
// Expected: Cleared title preface.
// Actual  : Title preface is still at 'xxx'.

https://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/browser/components/extensions/ext-windows.js#190 and
https://searchfox.org/mozilla-central/rev/15ce5cb2db0c85abbabe39a962b0e697c9ef098f/browser/components/extensions/ext-windows.js#221

should be changed to use: createData.titlePreface !== null

Comment 1

[Tushar Arora [:tushararora]]

Hey! I would like to work on this as my first bug.

Comment 2

[Bob Silverberg [:bsilverberg]]

(In reply to Tushar Arora from comment #1)
> Hey! I would like to work on this as my first bug.

That's great Tushar, thanks for your interest.

If you haven't built Firefox before, that's the place to start. The simplest way to do that is explained here:

https://developer.mozilla.org/en-US/docs/Artifact_builds

Once you have a working build, you can make the changes to the code as described in the bug description above. We will also need an existing test to be updated to prove that this change works. The test in question can be found at http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/browser/components/extensions/test/browser/browser_ext_windows.js.

Let me know if you run into any problems or have any questions.

Comment 3

[Tushar Arora [:tushararora]]

Attachment: title-preface-fix.patch

Attaching the patch, please review.

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8913314 [details] [diff] [review]
title-preface-fix.patch

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

Great work, Tushar, thank you for the patch.

Everything in here looks excellent, but looking at the code now I realize that we do not have test coverage for titlePreface being null. I.e., we do not have a test where we set a titlePreface and then call windows.update and pass nothing into the titlePreface property of the updateInfo object, and then assert that the preface has not changed. Do you mind updating the patch to add a test for that case as well?

Comment 5

[Tushar Arora [:tushararora]]

Hey, thanks Bob! Do you think instead of using !== we should use != to check for undefined as well as null?

Comment 6

[Bob Silverberg [:bsilverberg]]

(In reply to Tushar Arora from comment #5)
> Hey, thanks Bob! Do you think instead of using !== we should use != to check
> for undefined as well as null?

No, I think it's fine to use !== null, because the framework will pass a null if nothing is passed into the options object. This will need to go to a final review by a peer, so that may be wrong, but for now I think it's fine.

Comment 7

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8913314 [details] [diff] [review]
title-preface-fix.patch

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

Clearing my review flag for now so you can re-add it when the patch is ready for another review.

Comment 8

[Tushar Arora [:tushararora]]

Attachment: title-preface-fix-updated-tests.patch

Updated the patch with one more test for empty titlePreface. Please review.

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8918178 [details] [diff] [review]
title-preface-fix-updated-tests.patch

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

This looks great, thanks Tushar. I will flag one of the WebExtensions peers for a final review.

Comment 10

[Andreas Wagner [:TheOne] [use NI]]

Shane, setting check-in needed, we assume this is ready to land, given the r+.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad69d5466d3c
Fix browser.windows.update to clear titlePreface if it is passed empty. r=bsilverberg, r=mixedpuppy

Comment 12

[Dorel Luca [:dluca]]

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

Comment 13

[Victor Carciu]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!