Closed Bug 1464102 Opened 6 years ago Closed 6 years ago

Fix support for todo from within ContentTasks and add support for todo_is

Categories

(Firefox :: General, enhancement)

Product:
Firefox
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(5 files)

Bug 1464102 - Add message listener for content-task:test-todo so todo from within ContentTask will work.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1464102 - Add support for todo_is within ContentTask.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1464102 - Uncomment the shippingOptions portion of browser_request_serialization now that todo_is is supported.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1464102 - Change the todos in browser_siteData.js to is() now that todo is working and the conditions are passing.
59 bytes, text/x-review-board-request
johannh
: review+
Details
Bug 1464102 - Update the pageError.js test now that content-task.js has been updated.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
Bug 1236991 attempted to add support for todo() from within ContentTask but missed adding the event listener for the message.

While fixing this I'll also add support for todo_is() which is needed to uncomment the code at https://searchfox.org/mozilla-central/rev/dc6d85680539cb7e5fe2843d38a30a0581bfefe1/browser/components/payments/test/browser/browser_request_serialization.js#38-51
Comment on attachment 8980344 [details]
Bug 1464102 - Add message listener for content-task:test-todo so todo from within ContentTask will work.

https://reviewboard.mozilla.org/r/246512/#review252602

D'oh.
Attachment #8980344 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8980345 [details]
Bug 1464102 - Add support for todo_is within ContentTask.

https://reviewboard.mozilla.org/r/246514/#review252604
Attachment #8980345 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8980346 [details]
Bug 1464102 - Uncomment the shippingOptions portion of browser_request_serialization now that todo_is is supported.

https://reviewboard.mozilla.org/r/246516/#review252650

Thanks

::: browser/components/payments/test/browser/browser_request_serialization.js:41
(Diff revision 1)
>      ok(state, "got request store state");
>  
> -    // let expected = details;
> -    // let actual = state.request.paymentDetails;
> -    // if (expected.shippingOptions) {
> -    //   is(actual.shippingOptions.length, expected.shippingOptions.length,
> +    // The following test cases are conditionally todo because
> +    // the spec currently does not state the shippingOptions
> +    // should be null when requestShipping is not set. A future
> +    // spec change (bug 1436903 comments 7-12) will fix this.

Unrelated to this patch… would you mind following up on that spec issue and and/or getting a new bug filed for it.
Attachment #8980346 - Flags: review?(MattN+bmo) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43b864db2e34
Add message listener for content-task:test-todo so todo from within ContentTask will work. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/3c227d911b2f
Add support for todo_is within ContentTask. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/460cd874757f
Uncomment the shippingOptions portion of browser_request_serialization now that todo_is is supported. r=MattN
Comment on attachment 8980689 [details]
Bug 1464102 - Change the todos in browser_siteData.js to is() now that todo is working and the conditions are passing.

https://reviewboard.mozilla.org/r/246858/#review252962

Note that these were todo'd because the assertion was intermittent (I didn't understand that todo in ContentTasks was broken and should actually fail). I'd be willing to let this go intermittent again for checking if the problem persists.

Thanks!
Attachment #8980689 - Flags: review?(jhofmann) → review+
(In reply to Johann Hofmann [:johannh] from comment #14)
> Note that these were todo'd because the assertion was intermittent (I didn't
> understand that todo in ContentTasks was broken and should actually fail).
> I'd be willing to let this go intermittent again for checking if the problem
> persists.

Yeah, I'll comment in the original intermittent bug saying that we're going to try this again now.
Flags: needinfo?(jaws)
Comment on attachment 8980690 [details]
Bug 1464102 - Update the pageError.js test now that content-task.js has been updated.

https://reviewboard.mozilla.org/r/246860/#review252968

Update looks good, thanks!
Attachment #8980690 - Flags: review?(jdescottes) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9eff44581f49
Add message listener for content-task:test-todo so todo from within ContentTask will work. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/b9d9ed3b2ee1
Add support for todo_is within ContentTask. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d576bdf165b7
Uncomment the shippingOptions portion of browser_request_serialization now that todo_is is supported. r=MattN
https://hg.mozilla.org/integration/autoland/rev/9675ad8f3c22
Change the todos in browser_siteData.js to is() now that todo is working and the conditions are passing. r=johannh
https://hg.mozilla.org/integration/autoland/rev/9c6b035eedf6
Update the pageError.js test now that content-task.js has been updated. r=jdescottes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: