Closed
Bug 1342867
Opened 8 years ago
Closed 8 years ago
Label the runnable of ScrollOnFocusEvent in nsTextControlFrame.cpp
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: TYLin, Assigned: kuoe0.tw)
References
Details
(Whiteboard: [QDL][TDC-MVP][LAYOUT])
User Story
See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.
Attachments
(1 file)
All calls to NS_DispatchTo(Main|Current)Thread under layout/forms.
http://searchfox.org/mozilla-central/search?q=NS_DispatchTo(Main|Current)Thread&case=false®exp=true&path=layout%2Fforms
Reporter | ||
Updated•8 years ago
|
Component: Layout → Layout: Form Controls
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8844820 [details]
Bug 1342867 - Label the runnable of ScrollOnFocusEvent.
Hi Bevis, can you take a look for this patch? Is it the right way to use the API? Thanks!
Flags: needinfo?(btseng)
Attachment #8844820 -
Flags: feedback?(btseng)
Updated•8 years ago
|
Assignee: nobody → kuoe0
Status: NEW → ASSIGNED
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8844820 [details]
Bug 1342867 - Label the runnable of ScrollOnFocusEvent.
https://reviewboard.mozilla.org/r/118150/#review119972
f=me after the following issues are addressed.
Thanks!
::: layout/forms/nsTextControlFrame.cpp:665
(Diff revision 1)
> const bool isFocusedRightNow = ourSel == caretSelection;
> if (!isFocusedRightNow) {
> // Don't scroll the current selection if we've been focused using the mouse.
> uint32_t lastFocusMethod = 0;
> nsIDocument* doc = GetContent()->GetComposedDoc();
> if (doc) {
mContent is accessible from this class so we can simplify these 2 lines as:
if (nsIDocument* doc = mContent->GetComposedDoc()) {
and reuse mContent in your modification.
::: layout/forms/nsTextControlFrame.cpp:673
(Diff revision 1)
> fm->GetLastFocusMethod(doc->GetWindow(), &lastFocusMethod);
> }
> }
> if (!(lastFocusMethod & nsIFocusManager::FLAG_BYMOUSE)) {
> RefPtr<ScrollOnFocusEvent> event = new ScrollOnFocusEvent(this);
> - nsresult rv = NS_DispatchToCurrentThread(event);
> + nsresult rv = GetContent()->OwnerDoc()->Dispatch("ScrollOnFocusEvent",
*NS_DispatchToCurrentThread* here means the runnable to be dispatched will be run either on main thread or off main thread.
But the runnable we'd like to label are the one to the main thread.
If NS_DispatchToCurrentThread is always equal to NS_DispatchToMainThread, then we are fine. Otherwise, these change is applicable when NS_IsMainThread() is true and we have to keep the NS_DispatchToCurrentThread() for the off-main thread use case.
In addition, this line can we revise as:
nsresult rv = mContent->OwnerDoc()->Dispatch("ScrollOnFocusEvent", ...);
Attachment #8844820 -
Flags: review+
Comment 4•8 years ago
|
||
Comment on attachment 8844820 [details]
Bug 1342867 - Label the runnable of ScrollOnFocusEvent.
Not familiar with moz-review. Update my flags manually.
Flags: needinfo?(btseng)
Attachment #8844820 -
Flags: review+
Attachment #8844820 -
Flags: feedback?(btseng)
Attachment #8844820 -
Flags: feedback+
Assignee | ||
Comment 5•8 years ago
|
||
After scanning the codebase in layout/form, ScrollOnFocusEvent is the only one runnable we should label.
Summary: Label runnables in layout/forms/ → Label runnables of ScrollOnFocusEvent in nsTextControlFrame.cpp
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Summary: Label runnables of ScrollOnFocusEvent in nsTextControlFrame.cpp → Label the runnable of ScrollOnFocusEvent in nsTextControlFrame.cpp
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8844820 -
Flags: review?(ehsan)
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8844820 -
Flags: review?(ehsan) → review?(dholbert)
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8844820 [details]
Bug 1342867 - Label the runnable of ScrollOnFocusEvent.
https://reviewboard.mozilla.org/r/118150/#review126010
r=me with nits addressed:
::: layout/forms/nsTextControlFrame.cpp:678
(Diff revision 4)
> - nsresult rv = NS_DispatchToCurrentThread(event);
> + RefPtr<ScrollOnFocusEvent> eventKeeper(event);
> + nsresult rv = GetContent()->OwnerDoc()->Dispatch("ScrollOnFocusEvent",
> + TaskCategory::Other,
> + event.forget());
> if (NS_SUCCEEDED(rv)) {
> - mScrollEvent = event;
> + mScrollEvent = eventKeeper;
> }
As in the other bug, let's not introduce this "eventKeeper" extra variable. Just pass do_AddRef(event) into the Dispatch call.
(And then you can restore the "mScrollEvent" assignment to what it was before -- no need for this patch to modify that line.)
::: layout/forms/nsTextControlFrame.cpp:679
(Diff revision 4)
> + nsresult rv = GetContent()->OwnerDoc()->Dispatch("ScrollOnFocusEvent",
> + TaskCategory::Other,
s/GetContent()/mContent/
(And deindent subsequent lines accordingly)
Attachment #8844820 -
Flags: review?(dholbert) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8844820 [details]
Bug 1342867 - Label the runnable of ScrollOnFocusEvent.
Carry r+ from comment 10.
Attachment #8844820 -
Flags: review+
Comment 15•8 years ago
|
||
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88dd8824245d
Label the runnable of ScrollOnFocusEvent. r=bevistseng,dholbert
Keywords: checkin-needed
Comment 16•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in
before you can comment on or make changes to this bug.
Description
•