Closed Bug 1342867 Opened 3 years ago Closed 3 years ago

Label the runnable of ScrollOnFocusEvent in nsTextControlFrame.cpp

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set

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)

Component: Layout → Layout: Form Controls
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 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+
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
Summary: Label runnables of ScrollOnFocusEvent in nsTextControlFrame.cpp → Label the runnable of ScrollOnFocusEvent in nsTextControlFrame.cpp
Attachment #8844820 - Flags: review?(ehsan)
Blocks: 1347815
No longer blocks: 1347815
Attachment #8844820 - Flags: review?(ehsan) → review?(dholbert)
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 on attachment 8844820 [details]
Bug 1342867 - Label the runnable of ScrollOnFocusEvent.

Carry r+ from comment 10.
Attachment #8844820 - Flags: review+
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88dd8824245d
Label the runnable of ScrollOnFocusEvent. r=bevistseng,dholbert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88dd8824245d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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.