87348 - Relying on cached text length too heavily

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Blake Ross]

In nsGfxTextControlFrame2.cpp, ::GetTextLength:

  // We should probably invalidate the cached string on more
  // than just selection changes; maybe we should listen
  // for editor transactions etc.
  nsString *str = GetCachedString();
  if (str)
  {
    *aTextLength = str->Length();
    return NS_OK;
  }

However, we need to retrieve the text length now for whenever the urlbar is 
focused, and thus even though the page/url may have changed, we're still relying 
on the cached value.  Not sure how we'll get around this, but we need it for 
37587, which is wanted for rtm.

Comment 1

[Blake Ross]

Attachment: invalidate when setting new value?

Comment 2

[Blake Ross]

Simon/Mike, you guys touched this area frequently: is this a bad place to 
invalidate?

Comment 3

[rubydoo123]

I'm not sure that this will be prime for 9.3, this may well be very significant
change. asking smfr and kin for comment

Comment 4

[Blake Ross]

?

If my fix is okay, it's a one line change...

Comment 5

[kinmoz]

Your patch will not fix all cases ... we will still have the same problem 
if SetTextControlFrameState() is never called. (ie the value attribute is never 
set on the textarea, but the user types in the text widget.

I think we should just remove GetCachedString() entirely and make 
GetTextLength() call GetTextControlFrameState(). If there is a cached string, 
GetTextControlFrameState() will return it, if we have an editor, then it will 
give the current value back.

Comment 6

[Blake Ross]

Sounds good to me.

Comment 7

[Blake Ross]

(By the way, it still seems to work fine if the user types the value...I won't 
bother investigating why, though)

Comment 8

[Blake Ross]

Attachment: is this at all what you had in mind?

Comment 9

[Blake Ross]

Attachment: full patch

Comment 10

[Blake Ross]

Actually, I suppose it's still necessary to free mCachedState, so...do we want 
to keep InvalidateCachedState?

Comment 11

[Blake Ross]

Attachment: I like the ``one of these has to be right'' approach

Comment 12

[Blake Ross]

Kin, can you take a look at these and pick one or provide your own? ;-)  37587 
depends on this, and that's wanted on the branch.

Comment 13

[kinmoz]

sr=[e-mail] on the 06/25/01 20:06 patch.

This *shouldn't* break restoration of form text widget values when going 
forward/backwards in the browser history, but I'm a paranoid kind of guy, so can 
you just make sure that stuff still works before checking in?

Comment 14

[Blake Ross]

Restoration works fine for me.

smfr, r=?

Comment 15

[Simon Fraser [no longer active]]

What performance implications does this have?

Comment 16

[Blake Ross]

As far as I can tell, nothing. GetTextControlFrameState will still return the 
cached state when possible/necessary.  kin, correct me if i'm wrong...

Comment 17

[Blake Ross]

kin?

Comment 18

[Simon Fraser [no longer active]]

kin is on vacation

Comment 19

[Blake Ross]

Argh.  When will he be back?

Anyways, I'm fairly confident this has little effect on performance...It does 
almostthe same thing.  The difference is that we no longer call GetCachedString 
first, since GetTextControlFrameState will give us a cached string if one is 
available...

Comment 20

[Blake Ross]

-> me

Comment 21

[Simon Fraser [no longer active]]

I'll take your word for it. r=sfraser

Comment 22

[John Gaunt (redfive)]

adding PDT+, bug 37587 that depends on this bug is needed in the branch for 
accessibility purposes.

Comment 23

[chris hofmann]

paw/jrgm, do we have anything in the performance tests that could help
answer sfraser's questions?

Comment 24

[John Morrison]

I don't know if we could get a meaningful measure of this. This is rarely 
called when running page loading tests (actually never) and rarely called when 
opening a new browser window (i.e., only when the focus is set in the Urlbar).

Although I will make one correction to Blake's comments above (or at least this 
is what it looks like to me). GetTextControlFrameState would return the 
mCachedState if it is available except that it seems to always pass an earlier 
test and takes a different branch, so it never gets to the test for 
mCachedState.

Comment 25

[Blake Ross]

Why would that be? The tests in GetCachedState and GetTextControlFrame seem to 
be the same...

Is kin back yet?

Comment 26

[John Gaunt (redfive)]

This has been checked into the 0.9.2 branch on wed 4 july.

Comment 27

[lchiang]

Updating status whiteboard to reflect that this bug is open for the trunk.

Comment 28

[Blake Ross]

Fixed everywhere

Comment 29

[kinmoz]

Ok, I'm back from vacation ... I know this was checked in already, but just 
wanted to followup with some of the questions.

John is correct, a call to GetTextControlFrameState() will always retrieve the 
value from the editor if an editor is available, even if mCachedState is not 
null.

It looks like prior to blake's checkin, mCachedState was used for 2 purposes:

  1. A temporary storage place for the text widget's value if
     SetTextControlFrameState() was ever called before an editor was
     available. This happens when going forward or back in the browser
     because the history mechanism restores widget values before the
     first reflow ever occurs for the page.

  2. A quick way to cache the value for repeated calls to GetTextLength().

Blake removed #2 with his patch. For people who are concerned about the 
performance impact of this change, it should be noted that the only way to 
trigger a call to GetTextLength() is to use the textLength attribute on the 
nsIDOMNSHTMLInputElement interface, which no one seems to use except the urlbar 
code.

Comment 30

[sujay]

Blake, can you please verify this one and mark VERIFIED-FIXED?

thanks.

Comment 31

[Blake Ross]

Marking verified.