1023557 - At 20 chars, FrameMetrics::mContentDescription is often too short to be useful

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Bug 1015278 limited the length of FrameMetrics::mContentDescription to 20.

Unfortunately, 20 characters is often not enough to give us the information we need.

For example, the content description

  div@b2167040 class="scrollable"

gets truncated to

  div@b205d040 class=

The key piece of information - the class name of the div which allows one to identify which of the many divs in the DOM we are talking about - is missing.

Unfortunately, bumping the length a bit, say to 30, doesn't help too much, either. Consider another example:

  html@afa48200 sizemode="fullscreen" windowtype="navigator:browser" id="shell" xmlns="http://www.w3.org/1999/xhtml " chromehidden="menubar toolbar location directories status "

gets truncated to

  html@afa48200 sizem

Here, the key piece of information is

  id="shell"

but that's fairly far in the string, so bumping the limit a bit won't result in keeping it.

It seems we either need to build the content description string more cleverly (i.e. put the key information at the front), or return to using dynamically-sized strings and somehow make them work with shared memory.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

It looks like various ns*String classes are IPC-friendly. At least they have read/write functions in ipc/glue/IPCMessageUtils.h. So maybe we can replace the const char with one of those string classes and that would solve this problem as well as keeping it IPC-safe.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

If I'm reading https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings right then nsCString is probably the most appropriate thing to use.

Comment 3

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> It looks like various ns*String classes are IPC-friendly. At least they have
> read/write functions in ipc/glue/IPCMessageUtils.h. So maybe we can replace
> the const char with one of those string classes and that would solve this
> problem as well as keeping it IPC-safe.

If by "IPC-safe" you just mean "have read/write functions for sending over IPDL", then std::string - the thing we used initially - was IPC-safe as well.

The reason we had to abandon std::string was that it was not safe to copy the raw bytes into shared memory. Is that safe to do for nsCString?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

D'oh! You're right, I confused myself. It's probably unsafe to simply copy nsCString into shared memory as well, for the same reason that std::string is unsafe.

Comment 5

[Botond Ballo [:botond]]

As dicussed with Kats on IRC, a simple solution is to move mContentDescription from FrameMetrics to ContainerLayer, and change its type back to std::string. (Unlike FrameMetrics, ContainerLayer does not have to be shmem-safe.)

Comment 6

[Botond Ballo [:botond]]

Attachment: Move content description from FrameMetrics to ContainerLayer

Less manual string manipulation. More useful information APZC tree logs. Win-win from where I'm standing.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8465715 [details] [diff] [review]
Move content description from FrameMetrics to ContainerLayer

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

::: gfx/layers/FrameMetrics.h
@@ +400,5 @@
>    {
>      return mScrollGeneration;
>    }
>  
> +  void SetContentDescription(const std::string& aContentDescription);

Remove this?

Comment 8

[Benoit Girard (:BenWa)]

Comment on attachment 8465715 [details] [diff] [review]
Move content description from FrameMetrics to ContainerLayer

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

Cool. Maybe this field would be even better suited in the Layer base class.

::: gfx/layers/Layers.h
@@ +1771,5 @@
>    }
>  
> +  void SetContentDescription(const std::string& aContentDescription)
> +  {
> +    if (mContentDescription == aContentDescription) {

Keep in mind that you're not doing string equality here so you might risk sending the same string.

Comment 9

[Botond Ballo [:botond]]

Attachment: Move content description from FrameMetrics to ContainerLayer

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)

> > +  void SetContentDescription(const std::string& aContentDescription);
> 
> Remove this?

Fixed. Thanks for catching it!

Comment 10

[Botond Ballo [:botond]]

(In reply to Benoit Girard (:BenWa) from comment #8)
> Maybe this field would be even better suited in the Layer base class.

I will move it, along with mBackgroundColor, to Layer as part of the multi-layer-apz work.

> ::: gfx/layers/Layers.h
> > +  void SetContentDescription(const std::string& aContentDescription)
> > +  {
> > +    if (mContentDescription == aContentDescription) {
> 
> Keep in mind that you're not doing string equality here so you might risk
> sending the same string.

I am doing string equality. std::string has an overloaded operator== which compares the contents.

Comment 11

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=bd91dc90bc9a

Comment 12

[Botond Ballo [:botond]]

Attachment: bug1023557.patch

I forgot to remove the field FrameMetrics::mContentDescription itself, leading to Werrors. Fixed, carrying r+.

ReTrying: https://tbpl.mozilla.org/?tree=Try&rev=fb712b871d90

Comment 13

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71b50829a272

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71b50829a272

Comment 15

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

Unless there's some hidden reason why we can't use Gecko strings here, we should be.

Comment 16

[Botond Ballo [:botond]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Unless there's some hidden reason why we can't use Gecko strings here, we
> should be.

The logging framework in gfx/2d/Logging.h uses std::string and std::stringstream. (I extended this framework in bug 958596, but what was there before already used std::string and std::stringstream.) The content description is printed by the APZC tree log, which uses this logging code.

Logging code in Layers.cpp and LayersLogging.cpp also uses std::string and std::stringstream, so we are being consistent with that.

Does it matter much which string class we use?

Comment 17

[Botond Ballo [:botond]]

Bug 967844 regressed this again; reopening.

Kats had an idea for how to fix this in https://bugzilla.mozilla.org/show_bug.cgi?id=967844#c70; let's track any progress on that, or any alternative ideas, here.

Comment 18

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #17)
> Bug 967844 regressed this again; reopening.
> 
> Kats had an idea for how to fix this in
> https://bugzilla.mozilla.org/show_bug.cgi?id=967844#c70; let's track any
> progress on that, or any alternative ideas, here.

On second thought, let's track this in a different bug, as this landed in mozilla34 and a new fix would land in mozilla35. I filed bug 1062411.