1342867 - Label the runnable of ScrollOnFocusEvent in nsTextControlFrame.cpp

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 1

[Tommy Kuo (away forever...)]

Attachment: Bug 1342867 - Label the runnable of ScrollOnFocusEvent.

Review commit: https://reviewboard.mozilla.org/r/118150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118150/

Comment 2

[Tommy Kuo (away forever...)]

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!

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

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", ...);

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Not familiar with moz-review. Update my flags manually.

Comment 5

[Tommy Kuo (away forever...)]

After scanning the codebase in layout/form, ScrollOnFocusEvent is the only one runnable we should label.

Comment 6

[Tommy Kuo (away forever...)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118150/diff/1-2/

Comment 7

[Tommy Kuo (away forever...)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118150/diff/2-3/

Comment 8

[Tommy Kuo (away forever...)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118150/diff/3-4/

Comment 9

[Tommy Kuo (away forever...)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c5052da66855411dcd5a608eb77ea522e4b0127&selectedJob=83952711

Comment 10

[Daniel Holbert [: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)

Comment 11

[Tommy Kuo (away forever...)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118150/diff/4-5/

Comment 12

[Tommy Kuo (away forever...)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9620ac2969669756d6d9546f7bc9312d84ecb38b&selectedJob=86400180

Comment 13

[Tommy Kuo (away forever...)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118150/diff/5-6/

Comment 14

[Tommy Kuo (away forever...)]

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

Carry r+ from comment 10.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88dd8824245d
Label the runnable of ScrollOnFocusEvent. r=bevistseng,dholbert

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88dd8824245d