Closed
Bug 1299335
Opened 7 years ago
Closed 7 years ago
DeCOMtaminate nsIWidget even more
Categories
(Core :: Widget, defect, P3)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
(Whiteboard: tpi:+)
Attachments
(8 files, 1 obsolete file)
27.56 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
10.25 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
14.93 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
14.31 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
7.93 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
9.65 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
46.73 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
16.41 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
Continuing on from bug 1296993.
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: tpi:+
![]() |
||
Updated•7 years ago
|
Priority: P2 → P3
![]() |
Assignee | |
Comment 1•7 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•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 2•7 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•7 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•7 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•7 years ago
|
||
This patch changes it from |NS_IMETHOD| to |virtual MOZ_MUST_USE nsresult|.
Attachment #8819718 -
Flags: review?(mstange)
![]() |
Assignee | |
Comment 6•7 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•7 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•7 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•7 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•7 years ago
|
Attachment #8819816 -
Attachment is obsolete: true
Attachment #8819816 -
Flags: review?(mstange)
Updated•7 years ago
|
Attachment #8819715 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8819716 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8819717 -
Flags: review?(mstange) → review+
Comment 10•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8819734 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8819817 -
Flags: review?(mstange) → review+
Updated•7 years ago
|
Attachment #8819818 -
Flags: review?(mstange) → review+
![]() |
Assignee | |
Comment 11•7 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 12•7 years ago
|
||
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•7 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.
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87e9a25f3d53 https://hg.mozilla.org/mozilla-central/rev/a75e4a674e23 https://hg.mozilla.org/mozilla-central/rev/fe8165b5dbe9 https://hg.mozilla.org/mozilla-central/rev/19292462b6f5 https://hg.mozilla.org/mozilla-central/rev/6e0238046725 https://hg.mozilla.org/mozilla-central/rev/bf2c019eaf93 https://hg.mozilla.org/mozilla-central/rev/1da69ae6f724 https://hg.mozilla.org/mozilla-central/rev/0922b81af274
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 15•7 years ago
|
||
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.
Description
•