DeCOMtaminate nsIWidget even more

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

(Blocks 1 bug)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox53 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(8 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Continuing on from bug 1296993.
Priority: -- → P2
Whiteboard: tpi:+

Updated

3 years ago
Priority: P2 → P3
(Assignee)

Comment 1

2 years ago
Specifically: OnDefaultButtonLoaded, AttachNativeKeyEvent, BeginMoveDrag,
BeginResizeDrag, GetAttention. These are all fallible functions whose result is
always checked.

The patch also moves some trivial function definitions from nsBaseWidget.cpp to
nsBaseWidget.h, and removes the android BeginResizeDrag() because it can
use the nsBaseWidget one.
Attachment #8819714 - Flags: review?(jmathies)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
This patch does the following.

- Removes the return value, because none of the call sites check it.

- Removes the empty implementations from several nsIWidget instances, because
  they can use the nsBaseWidget one.
Attachment #8819715 - Flags: review?(mstange)
(Assignee)

Comment 3

2 years ago
This patch removes its return value, because none of the call sites check it
except for one non-vital assertion.
Attachment #8819716 - Flags: review?(mstange)
(Assignee)

Comment 4

2 years ago
This patch does the following.

- Removes the return value, because none of the call sites check it.

- Removes the empty implementations from the android nsIWidget instance,
  because it can use the nsBaseWidget one.
Attachment #8819717 - Flags: review?(mstange)
(Assignee)

Comment 5

2 years ago
This patch changes it from |NS_IMETHOD| to |virtual MOZ_MUST_USE nsresult|.
Attachment #8819718 - Flags: review?(mstange)
(Assignee)

Comment 6

2 years ago
This patch changes them from |NS_IMETHOD| to |virtual void| because the return
values are never used.
Attachment #8819734 - Flags: review?(mstange)
(Assignee)

Comment 7

2 years ago
This patch changes them from |NS_IMETHOD| to |virtual void| because the return
values are never used.
Attachment #8819816 - Flags: review?(mstange)
(Assignee)

Comment 8

2 years ago
This patch changes them from |NS_IMETHOD| to |virtual void| because every
implementation of these functions always returns |NS_OK|.
Attachment #8819817 - Flags: review?(mstange)
(Assignee)

Comment 9

2 years ago
This patch changes it from |NS_IMETHOD| to |virtual void| because every
implementation of these functions always returns |NS_OK|.
Attachment #8819818 - Flags: review?(mstange)
(Assignee)

Updated

2 years ago
Attachment #8819816 - Attachment is obsolete: true
Attachment #8819816 - Flags: review?(mstange)
Attachment #8819715 - Flags: review?(mstange) → review+
Attachment #8819716 - Flags: review?(mstange) → review+
Attachment #8819717 - Flags: review?(mstange) → review+
Comment on attachment 8819718 [details] [diff] [review]
(part 5) - Streamline nsIWidget::StartPluginIME

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

Is there a caller that checks the result? If not, why did you choose to make this one MOZ_MUST_USE nsresult instead of void?

r=me either way, it's an improvement regardless
Attachment #8819718 - Flags: review?(mstange) → review+
Attachment #8819734 - Flags: review?(mstange) → review+
Attachment #8819817 - Flags: review?(mstange) → review+
Attachment #8819818 - Flags: review?(mstange) → review+
(Assignee)

Comment 11

2 years ago
> (part 5) - Streamline nsIWidget::StartPluginIME
> 
> Is there a caller that checks the result?

Good question. There is one checked call, in nsPluginInstanceOwner::ProcessEvent().
Comment on attachment 8819714 [details] [diff] [review]
(part 1) - Change some nsIWidget function return values from |NS_IMETHOD| to |virtual MOZ_MUST_USE nsresult|

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

lgtm
Attachment #8819714 - Flags: review?(jmathies) → review+

Comment 13

2 years ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e9a25f3d53
(part 1) - Change some nsIWidget function return values from |NS_IMETHOD| to |virtual MOZ_MUST_USE nsresult|. r=jimm.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a75e4a674e23
(part 2) - Streamline nsIWidget::SetIcon. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8165b5dbe9
(part 3) - Streamline nsIWidget::SetParent. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/19292462b6f5
(part 4) - Streamline nsIWidget::HideWindowChrome. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e0238046725
(part 5) - Streamline nsIWidget::StartPluginIME. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2c019eaf93
(part 6) - Streamline nsIWidget::{Move,Resize}Client(). r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da69ae6f724
(part 7) - Streamline nsIWidget::{Move,Resize}. r=mstange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0922b81af274
(part 8) - Streamline nsIWidget::Enable. r=mstange.
(Assignee)

Updated

2 years ago
Blocks: 1325234
We've built 51 RC. Mark 51 won't fix.
You need to log in before you can comment on or make changes to this bug.