Closed Bug 908954 Opened 7 years ago Closed 3 months ago

Remove try…catch inside wrapWidgetEventHandler once bug 503244 is fixed

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
Firefox 77
Tracking Status
firefox77 --- fixed

People

(Reporter: MattN, Assigned: abhirav.kariya, Mentored)

References

()

Details

(Keywords: good-first-bug, Whiteboard: [Australis:M-][lang=js])

Attachments

(2 files, 9 obsolete files)

(Quoting Dão Gottwald [:dao] from comment bug 865916 comment 88)
> It seems like bug 862627 will happen eventually, so the workaround may not
> be worth it. But if you want to keep it, you should add a comment pointing
> to a bug so we'll eventually get rid of the workaround.

https://hg.mozilla.org/projects/ux/rev/5a7f22213ce9#l3.145
Mentor: manishearth
Whiteboard: [Australis:M-] → [Australis:M-][good first bug]
Those interested in takign this bug: Leave a comment here and ask me if you need any further help.

The relevant code is here[1].

 [1]: http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#2176
Whiteboard: [Australis:M-][good first bug] → [Australis:M-][good first bug][lang=js]
Attached patch bug-908954 (obsolete) — Splinter Review
Please check!
Attachment #8479366 - Flags: review?(manishearth)
Comment on attachment 8479366 [details] [diff] [review]
bug-908954

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

In the future, ask me for feedback -- MattN should probably review this :)

::: browser/components/customizableui/CustomizableUI.jsm
@@ -2181,5 @@
>      aWidget[aEventName] = function(...aArgs) {
> -      // Wrap inside a try...catch to properly log errors, until bug 862627 is
> -      // fixed, which in turn might help bug 503244.
> -      try {
> -        // Don't copy the function to the normalized widget object, instead

These comments probably shouldn't be removed.

@@ -2189,1 @@
>          return aWidget.implementation[aEventName].apply(aWidget.implementation,

Nit: de-indent
Attachment #8479366 - Flags: review?(manishearth)
Attached patch bug908954 (obsolete) — Splinter Review
Please check!!
Attachment #8479396 - Flags: review?(manishearth)
Comment on attachment 8479366 [details] [diff] [review]
bug-908954

In the future, mark old patches as obsolete, please :)
Attachment #8479366 - Attachment is obsolete: true
Comment on attachment 8479396 [details] [diff] [review]
bug908954

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

::: browser/components/customizableui/CustomizableUI.jsm
@@ +2178,5 @@
>        aWidget[aEventName] = null;
>        return;
>      }
>      aWidget[aEventName] = function(...aArgs) {
> +      // Don't copy the function to the normalized widget object, instead             // keep it on the original object provided to the API so that        

nit: This line isn't indented properly, also there's some extra whitespace at the end

@@ +2182,5 @@
> +      // Don't copy the function to the normalized widget object, instead             // keep it on the original object provided to the API so that        
> +      // additional methods can be implemented and used by the event
> +      // handlers
> +    return aWidget.implementation[aEventName].apply(aWidget.implementation,
> +			    aArgs);

Nit: The `aArgs` should be aligned with `aWidget` as it was previously.
Attachment #8479396 - Flags: review?(manishearth)
Attached file bug.908954 (obsolete) —
please check! (ignore previous submission )
Attachment #8479397 - Flags: review?(manishearth)
Attached patch bug.908954 (obsolete) — Splinter Review
sorry for taking so many attempts (new to all this)
Attachment #8479396 - Attachment is obsolete: true
Attachment #8479397 - Attachment is obsolete: true
Attachment #8479397 - Flags: review?(manishearth)
Attachment #8479398 - Flags: review?(manishearth)
Attachment #8479398 - Attachment is patch: true
Comment on attachment 8479398 [details] [diff] [review]
bug.908954

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

Fine if you take many attempts, I took a lot on my first bugs too :)

::: browser/components/customizableui/CustomizableUI.jsm
@@ +2179,5 @@
>        return;
>      }
>      aWidget[aEventName] = function(...aArgs) {
> +      // Don't copy the function to the normalized widget object, instead
> +      // keep it on the original object provided to the API so that        

Still have that extra whitespace at the end of the line :)
Attachment #8479398 - Flags: review?(manishearth)
Attached patch bug.908954 (obsolete) — Splinter Review
ty
Attachment #8479398 - Attachment is obsolete: true
Attachment #8479404 - Flags: review?(manishearth)
Comment on attachment 8479404 [details] [diff] [review]
bug.908954

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

This patch still has the whitespace, and also doesn't apply to the original file -- it only contains the indentation change (not the try-catch change)
Attachment #8479404 - Flags: review?(manishearth)
Attached patch bug-908954.patch (obsolete) — Splinter Review
Removed try..catch and respective comment, with proper indentation.
Attachment #8479404 - Attachment is obsolete: true
Attachment #8479424 - Flags: review?(MattN+bmo)
Attachment #8479424 - Flags: feedback?(manishearth)
Comment on attachment 8479424 [details] [diff] [review]
bug-908954.patch

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

Looks good to me, though the aArgs could be aligned with the aWidget (not too necessary)
Attachment #8479424 - Flags: feedback?(manishearth) → feedback+
Assignee: nobody → abhirav.kariya
Status: NEW → ASSIGNED
Comment on attachment 8479424 [details] [diff] [review]
bug-908954.patch

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

I'm no longer convinced it's a good idea to remove the try…catch since an exception in a widget's event handler shouldn't break the flow of callers in CustomizableUI.jsm. For example, if a widget implements onCreated and throws, we won't finish running buildWidget which means we won't call |aWidget.instances.set(aDocument, node);|. That doesn't seem like a good thing. Blair, what do you think? Maybe we should just remove the comment?
Attachment #8479424 - Flags: review?(MattN+bmo) → review?(bmcbride)
Comment on attachment 8479424 [details] [diff] [review]
bug-908954.patch

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

Good catch, MattN. My comment about this at bug 865916 comment 82 is no longer accurate, as we've since removed the other try/catches for these JS-implemented events (cf, bug 981418) and we're just relying on wrapWidgetEventHandler() to make them safe.

So yea, we should just remove the comment - keeping the try/catch and reportError() call.
Attachment #8479424 - Flags: review?(bmcbride) → review-
Alright, thanks.

Abhirav, could you make a patch that removes the first comment (Wrap...)? Sorry about the confusion, after this we'll find you a bug with something more interesting.
ok .. sorry I was inactive since 2 days .. ill submit as soon as possible
Attached patch Done (obsolete) — Splinter Review
please explain how to restore the firefox build ..
Attachment #8479424 - Attachment is obsolete: true
Attachment #8481769 - Flags: review?(manishearth)
Comment on attachment 8481769 [details] [diff] [review]
Done

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

This patch adds a try-catch block, instead of removing it. You probably aren't on a clean patch queue.

https://developer.mozilla.org/en/docs/Mercurial_FAQ

https://developer.mozilla.org/en/docs/Mercurial_Queues

Ping me in IRC if you want further help.
Attachment #8481769 - Flags: review?(manishearth) → feedback-
Attached patch patch.diff (obsolete) — Splinter Review
I hope this solves it.
Flags: needinfo?(manishearth)
Attachment #8570246 - Flags: superreview?(manishearth)
Attachment #8570246 - Flags: review?(jaws)
Attachment #8570246 - Flags: feedback?(manishearth)
Comment on attachment 8570246 [details] [diff] [review]
patch.diff

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

Looks good. Could you add a commit message too?

Instructions [here](https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F)
Attachment #8570246 - Flags: superreview?(manishearth)
Attachment #8570246 - Flags: feedback?(manishearth)
Attachment #8570246 - Flags: feedback+
Attached patch patch_1.diff (obsolete) — Splinter Review
Commit message has been added .
Flags: needinfo?(manishearth)
Attachment #8570428 - Flags: ui-review?
Attachment #8570428 - Flags: review?(manishearth)
Attachment #8570428 - Flags: feedback?(manishearth)
I don't see any commit message.

Are you using Git?
Attachment #8570428 - Attachment is obsolete: true
Attachment #8570428 - Flags: ui-review?
Attachment #8570428 - Flags: review?(manishearth)
Attachment #8570428 - Flags: feedback?(manishearth)
Comment on attachment 8570246 [details] [diff] [review]
patch.diff

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

This patch doesn't remove the try/catch wrapping, it just removes the comment referencing it.
Attachment #8570246 - Flags: review?(jaws)
yes I am using git.
Comment on attachment 8570246 [details] [diff] [review]
patch.diff

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

Ah, well then :)
Attachment #8570246 - Flags: review+
Attached patch patch_1.diffSplinter Review
added the commit message .
Attachment #8570246 - Attachment is obsolete: true
Attachment #8570686 - Flags: review?(manishearth)
Attachment #8570686 - Flags: feedback?(manishearth)
Attachment #8481769 - Attachment is obsolete: true
Attachment #8570686 - Flags: review?(manishearth)
Attachment #8570686 - Flags: review?(jaws)
Attachment #8570686 - Flags: feedback?(manishearth)
Attachment #8570686 - Flags: feedback+
hi, this failed to apply:

unable to find 'components/customizableui/CustomizableUI.jsm' for patching
1 out of 1 hunks FAILED -- saving rejects to file components/customizableui/CustomizableUI.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh patch_1.diff

could you take a look, thanks!
Flags: needinfo?(abhirav.kariya)
Keywords: checkin-needed
Keywords: good-first-bug
Whiteboard: [Australis:M-][good first bug][lang=js] → [Australis:M-][lang=js]

Hello, Can I submit the patch for this ?
Reading the above comments, I have to remove the following comment https://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#2857 ?
Thanks :)
Aarushi

Flags: needinfo?(manishearth)
Flags: needinfo?(MattN+bmo)

Yes, you can rebase the patch from comment 28.

Flags: needinfo?(manishearth)
Flags: needinfo?(abhirav.kariya)
Flags: needinfo?(MattN+bmo)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/a427b90947bf
Remove try…catch comment inside wrapWidgetEventHandler once bug 503244 is fixed r=MattN
Attachment #9141162 - Attachment description: Bug 908954 - Remove try…catch comment inside wrapWidgetEventHandler once bug 503244 is fixed r=MattN → Bug 908954 - Remove try…catch comment inside wrapWidgetEventHandler r=MattN
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 77
You need to log in before you can comment on or make changes to this bug.