If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Label the runnable of ScrollOnFocusEvent in nsTextControlFrame.cpp

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: TYLin, Assigned: kuoe0)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [QDL][TDC-MVP][LAYOUT])

User Story

See https://wiki.mozilla.org/Quantum/DOM#Labeling for the story.

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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&regexp=true&path=layout%2Fforms
Component: Layout → Layout: Form Controls
Comment hidden (mozreview-request)
(Assignee)

Comment 2

7 months 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)
Assignee: nobody → kuoe0
Status: NEW → ASSIGNED

Comment 3

7 months 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 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

7 months 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

7 months 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

7 months ago
Attachment #8844820 - Flags: review?(ehsan)
(Assignee)

Comment 9

7 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c5052da66855411dcd5a608eb77ea522e4b0127&selectedJob=83952711
Blocks: 1347815
No longer blocks: 1347815
(Assignee)

Updated

7 months ago
Attachment #8844820 - Flags: review?(ehsan) → review?(dholbert)

Comment 10

7 months 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

7 months ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9620ac2969669756d6d9546f7bc9312d84ecb38b&selectedJob=86400180
Keywords: checkin-needed
Comment hidden (mozreview-request)
(Assignee)

Comment 14

7 months ago
Comment on attachment 8844820 [details]
Bug 1342867 - Label the runnable of ScrollOnFocusEvent.

Carry r+ from comment 10.
Attachment #8844820 - Flags: review+

Comment 15

7 months 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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88dd8824245d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [QDL][TDC-MVP][LAYOUT]
You need to log in before you can comment on or make changes to this bug.