1087190 - [Text selection] Add debug logs to SelectionCarets

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

It will be easier to debug SelectionCarets with some logs.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Add debug log to SelectionCarets.

Morris,

If there're some critical points in SelectionCarets that should have a log print, feel free to let me know.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8509307 [details] [diff] [review]
Add debug log to SelectionCarets.

Review of attachment 8509307 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/SelectionCarets.cpp
@@ +1032,5 @@
>    nsRefPtr<SelectionCarets> self = static_cast<SelectionCarets*>(aSelectionCarets);
>    NS_PRECONDITION(aTimer == self->mLongTapDetectorTimer,
>                    "Unexpected timer");
>  
> +  SELECTIONCARETS_LOG_STATIC("SelectWord");

This long tap event would be fired if apzc is not available. for apzc case, we'll receive event from apzc and handle it at SelectionCarets::HandleEvent(). So, please add a log for apzc long tap as well and both log message should able to distinguish where is long tap coming from.

Comment 3

[Peter Chang[:pchang]]

Comment on attachment 8509307 [details] [diff] [review]
Add debug log to SelectionCarets.

Review of attachment 8509307 [details] [diff] [review]:
-----------------------------------------------------------------

I suggest to add the log for SelectWord, but the GetFrameName is required to define DEBUG_FRAME_DUMP.


111    SetSelectionDragState(true);
112    nsFrame* frame = static_cast<nsFrame*>(ptFrame);
113    nsresult rs = frame->SelectByTypeAtPoint(mPresShell->GetPresContext(), ptInFrame,
114                                             eSelectWord, eSelectWord, 0);
115 +  frame->GetFrameName(fName);
116 +  printf_stderr("peter SelectionCarets::SelectWord(%d) ptFrame %p  ptFrameyType %s ptInFrame %f %f flags 0x%08x\n",
117 +    __LINE__, frame, NS_ConvertUTF16toUTF8(fName).get(), (float)ptInFrame.x, (float)ptInFrame.y, flags);

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Add debug log to SelectionCarets. f=mtseng, r=roc

Addressed comment 2 and comment 3.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8510124 [details] [diff] [review]
Add debug log to SelectionCarets. f=mtseng, r=roc

Review of attachment 8510124 [details] [diff] [review]:
-----------------------------------------------------------------

Please use NSPR logging instead of rolling your own framework.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Add debug log to SelectionCarets. f=mtseng, r=roc (v2)

Use NSPR logging.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

To export NSPR_LOG_MODULES=SelectionCarets:5 and enable NSPR log on B2G, see this document.
https://developer.mozilla.org/en-US/Firefox_OS/Developing_Firefox_OS/Customizing_the_b2g.sh_script

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8510963 [details] [diff] [review]
Add debug log to SelectionCarets. f=mtseng, r=roc (v2)

Review of attachment 8510963 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!!!

Comment 9

[Peter Chang[:pchang]]

Comment on attachment 8510963 [details] [diff] [review]
Add debug log to SelectionCarets. f=mtseng, r=roc (v2)

Review of attachment 8510963 [details] [diff] [review]:
-----------------------------------------------------------------

With this patch, I could dump the selectioncaret log in B2G.

::: layout/base/SelectionCarets.cpp
@@ +941,5 @@
>                                         nsISelection* aSel,
>                                         int16_t aReason)
>  {
> +  SELECTIONCARETS_LOG("Reason=%d", aReason);
> +

Please also dump the collapse state here.

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use __FUNCTION__ and add log about collapsed. f=pchang

Peter, I'll fixup this patch to my main patch if it's look good to you.

Comment 11

[Peter Chang[:pchang]]

Comment on attachment 8512692 [details] [diff] [review]
Use __FUNCTION__ and add log about collapsed. f=pchang

Review of attachment 8512692 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8512692 [details] [diff] [review]
Use __FUNCTION__ and add log about collapsed. f=pchang

Review of attachment 8512692 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/SelectionCarets.cpp
@@ +950,4 @@
>    if (isCollapsed) {
>      SetVisibility(false);
>      return NS_OK;
>    }

This was removed in bug 1074736. I'll print the collapsed info in UpdateSelectionCarets()

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Add debug log to SelectionCarets. f=mtseng, f=pchang, r=roc (v3, carry r+)

Rebased.

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=09cd7da02ca9

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Add debug log to SelectionCarets. f=mtseng, f=pchang, r=roc (v4, carry r+)

s/kLogModuleName/kSelectionCaretsLogModuleName so that it wont't clash with
names in other cpp in unified build in the future.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/468517afd116

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/468517afd116