598833 - Consider storing intrinsic state in a direct member of Element

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

Right now IntrinsicState() is virtual, and for some kinds of elements expensive.

If we're willing to pay a word of space (2 words soon on 32-bit, I bet), we could store the state directly in Element.  Instead of computing the value on each get, what we would do is have a public getter that just returns the member and a protected UpdateIntrinsicState(), which computes the new value (via a virtual hook like ComputeIntrinsicState() or something), compares to the old value, and dispatches ContentStatesChanged notifications if they differ.  Then we replace all places where we dispatch ContentStatesChanged with an UpdateIntrinsicState() call.  This should be provably correct in terms of us continuing to notify every time we need to notify.

If we wanted to, we could even store the ESM state in the nodes, either as part of this bug or as a followup.

There are some niggly details like making sure that ContentStatesChanged only cares about Elements and possibly changing its signature to take Elements, as well as making sure that all not-inside-the-element things that notify about intrinsic state changes go through UpdateIntrinsicState() instead.  But the biggest sticking point is the memory bloat.  If we're ok with it, I can write up a patch right after we finish up with all the blockers, esp. if Mounir is not adding more ContentStatesChanged calls at that point.  ;)

Relevant size data, in bytes, without this change, in an opt build:

   Class         32-bit         64-bit
----------------------------------------
 nsINode           36             72
 nsIContent        40             80
 Element           40             80
 nsGenericElement  48             96

So on 32-bit we have plenty of space (well, 3 words) until we hit the size of a cache line for even nsGenericElement.  On 64-bit we're already over it with nsINode, and aren't pushing the second cache line yet.  The proposed change would basically add 8 bytes to Element (well, 4 on 32-bit for the moment, but I don't expect that to last).

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can you give a sense for how much time we spend in IntrinsicState in "bad" testcases? I.e. how much perf could we stand to gain here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Right now, a few percent at most (largely due to having one fewer virtual function call in SetAttr and having slightly faster RuleProcessorData construction).  If we want to do bug 598832, IntrinsicState() jumps to something like 10% of selector-matching time in the tests I've tried (which puts it at 3-4% of total testcase times for things like restyling the HTML5 spec).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Getting this to play with the LinkState stuff might be fun.  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

I suppose we could just add new API for us to explicitly trigger the link stuff as needed.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> I suppose we could just add new API for us to explicitly trigger the link stuff
> as needed.
If I'm understanding this all correctly, I think we already have something that would work: Link::SetLinkState.  This does happen to be a virtual call right now though.  I don't think it'd be too hard to hook this up with the LinkState stuff, but it certainly adds some complications.

Comment 6

[Mounir Lamouri (:mounir)]

I'm going to add bug that have patches playing with event states as a dependency.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I have this basically implemented.  I _think_ the code is cleaner, though there ended up being a bunch of form control (and object, sadly) edge cases.  It could probably be simplified more (e.g. we can push out the state update for attr changes so that we don't have to do extra state updates in Before/AfterSetAttr), but I figured we can do that in followups.

In the meantime, I did some performance tests.  Things are a wash on the slickspeed selector tests and on most of Talos.  There might be a 2% Txul win.  You can see a Talos compare at <http://perf.snarkfest.net/compare-talos/index.html?oldRevs=6e146150948f&newRev=8350990bad5d>. 

There's one regression on Talos: the 30% dromaeo dom-modify regression.  I've got that figured out and will do another try run with it fixed.

The good news is that this is a 7-8% win on the "recreate the frame tree but don't do layout on the single-page HTML spec" testcase and gets us closer to being able to do all of style resolution on |const Element*| and thus being able to parallelize it.

I'll post the patches once I get one remaining niggling a11y test failure fixd and do the try talos runs with dom-modify fixed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move IntrinsicState from nsIContent to Element.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Create basic infrastructure for letting the ESM store states on elements.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Store hover and active state directly on elements.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Store urltarget and dragover state directly on elements.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Store focus and focusring state directly on elements.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Get rid of nsEventStateManager::GetContentState.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Make IntrinsicState protected.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Do you have new performance results? I assume you fixed that dom-modify regression.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yes, http://perf.snarkfest.net/compare-talos/index.html?oldRevs=6e146150948f&newRev=6768c262ca9c is sort of the new result.  It's using the same build as the previous compare as the oldRevs and a built off tip for the newRev, so it shows sunspider regressions that are due to some JS changes in the meantime, but otherwise looks ok.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Create an explicit API to request link state updates. is a bit of a hack to make sure that we update link state sometime when doing style resolution, pending a better setup in followup bugs for determining when to resolve the link URI

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Store an Element pointer in Link.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Make unbinding a <select> update its validation state.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Changes to @form should notify the old default submit correctly.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Add dom::Element::UpdateState and use it in various places where elements update their own state.

This actually flips the switch to not recomputing state on every state get.  I'm sorry about the size of the patch; I couldn't figure out a way to do this incrementally.  Shawn, please review the link changes.  Mounir, please review the forms changes.  Olli, you get to review the rest plus those...  ;)

Comment 22

[Shawn Wilsher :sdwilsh]

Comment on attachment 532653 [details] [diff] [review]
part 8.  Create an explicit API to request link state updates.   is a bit of a hack to make sure that we update link state sometime when doing style resolution, pending a better setup in followup bugs for determining when to resolve the link URI

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

r=sdwilsh

::: content/base/src/nsGenericElement.cpp
@@ +810,5 @@
>  
>  void
> +Element::RequestLinkStateUpdate()
> +{
> +}

Can you add an assertion there to check the invariant that is documented:
This will make sure that if the element is a link at all then either NS_EVENT_STATE_VISITED or NS_EVENT_STATE_UNVISITED is set in mState...

And then every class that overrides this will also have to call Element::RequestLinkStateUpdate.

@@ +816,5 @@
> +void
> +Element::UpdateLinkState(nsEventStates aState)
> +{
> +  NS_PRECONDITION(!aState.HasAtLeastOneOfStates(~(NS_EVENT_STATE_VISITED | NS_EVENT_STATE_UNVISITED)),
> +                  "Unexpected link state bits");

This is OK to be fatal, right?  Might be worthwhile to make this MOZ_ASSERT (in mozilla/Util.h) instead.

Comment 23

[Shawn Wilsher :sdwilsh]

Comment on attachment 532655 [details] [diff] [review]
part 9.  Store an Element pointer in Link.

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

r=sdwilsh

::: content/base/src/Link.cpp
@@ +52,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +Link::Link(Element* aElement)

As much as it pains me to say this, style in this file is Element *aElement (which you got right everywhere else :D).

@@ +60,3 @@
>    , mHistory(services::GetHistoryService())
>  {
>  }

Add a MOZ_ASSERT(aElement) here since we don't ever want this to be null (bad things would happen like crashes later on).  I'd rather catch that right away rather than later when we try to use mElement.

@@ -546,5 @@
> -  }
> -
> -  nsCOMPtr<nsIContent> content(do_QueryInterface(this));
> -  NS_ABORT_IF_FALSE(content, "This must be able to QI to nsIContent!");
> -  return mContent = content;

Hurray for getting rid of a QI that could happen a lot!

::: toolkit/components/places/tests/cpp/mock_Link.h
@@ +52,5 @@
>    NS_DECL_ISUPPORTS
>  
>    mock_Link(void (*aHandlerFunction)(nsLinkState),
>              bool aRunNextTest = true)
> +  : mozilla::dom::Link(nsnull)

So, I really like the idea of adding the assertion to Link's constructor.  We should be able to pass 0x1 here (with a comment explaining why) so we can add that assertion.  It's that, or also mock out Element (which may have value, but that is your call).

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 532662 [details] [diff] [review]
part 12.  Add dom::Element::UpdateState and use it in various places where elements update their own state.

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

r=sdwilsh

::: content/base/public/Element.h
@@ +120,5 @@
> +   * Ask this element to update its state.  If aNotify is false, then
> +   * state change notifications will not be dispatched; in that
> +   * situation it is the caller's responsibility to dispatch them.
> +   */
> +  void UpdateState(PRBool aNotify);

nit: is the content module anti-bool?

::: content/base/src/Link.cpp
@@ +90,5 @@
>    // Per IHistory interface documentation, we are no longer registered.
>    mRegistered = false;
>  
> +  NS_ASSERTION(LinkState() == NS_EVENT_STATE_VISITED ||
> +               LinkState() == NS_EVENT_STATE_UNVISITED,

Switch this to MOZ_ASSERT please.  This should be fatal.

@@ +482,5 @@
> +  // want to call UpdateState, because that will call into LinkState()
> +  // and try to start off loads, etc.  But ResetLinkState is called
> +  // with aNotify false when things are in inconsistent states, so
> +  // we'll get confused in that situation.  Instead, just silently
> +  // update the link state on mElement.

Ah, I'm glad you rewrote this because it makes far more sense now!

Comment 25

[Boris Zbarsky [:bzbarsky]]

> Can you add an assertion there to check the invariant that is documented:

As discussed on IRC, not really.

> This is OK to be fatal, right?  Might be worthwhile to make this MOZ_ASSERT 

Yes, done (with NS_ABORT_IF_FALSE for now).

> As much as it pains me to say this, style in this file is Element *aElement
...
> Add a MOZ_ASSERT(aElement) here

Fixed.

> So, I really like the idea of adding the assertion to Link's constructor.

Not a problem; mock_Link.h implements its own Link constructor, since it shares no non-inline code with content/base.  So there is no problem with me adding an assert to the one in content/base/src.

> nit: is the content module anti-bool?

Nope.  I checked with smaug and will change all the args for UpdateState to bool.  I won't bother posting patches with that mechanical change.

> Switch this to MOZ_ASSERT please.  This should be fatal.

Done.

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 532657 [details] [diff] [review]
part 10.  Make unbinding a <select> update its validation state.

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

Comment 27

[Mounir Lamouri (:mounir)]

Comment on attachment 532659 [details] [diff] [review]
part 11.  Changes to @form should notify the old default submit correctly.

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

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +2900,1 @@
>    bool hadForm = mForm;

Did you meant to move this code before |bool hadForm = mForm|? I presume the behavior might change...

::: layout/reftests/forms/reftest.list
@@ +52,5 @@
>  == select-dynamic-boguskids.html select-boguskids-ref.html
>  
> +== default-submit-1.html default-submit-ref.html
> +== default-submit-2.html default-submit-ref.html
> +

Could you add those tests to layout/reftests/css-default/ instead? You can put them in submit-button/ or create a new directory.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> Did you meant to move this code before |bool hadForm = mForm|?

Yes.  That movement is what fixes the default-submit-1 test.  Without that change, we did not notify the submit that used to be default for <form id="two"> that it was no longer default (because hadForm was true, so we passed false for aNotify when adding to the new form).  The !hadForm test there is just meant to catch the aBindToTree case when we already have a form (i.e. are coming from the parser).

> Could you add those tests to layout/reftests/css-default/ instead?

Done; put them in submit-button.

Comment 29

[Mounir Lamouri (:mounir)]

Comment on attachment 532659 [details] [diff] [review]
part 11.  Changes to @form should notify the old default submit correctly.

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

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +2900,1 @@
>    bool hadForm = mForm;

I would have ask to rename |hadForm| to |notify| but I see that you changed that in part 12.

Comment 30

[Mounir Lamouri (:mounir)]

Boris, the plan is to fix this bug for the next branch?
I have a patch that might land with states handling (<input type=number>) and a group of students I'm mentoring might have one too (<meter>)).

Comment 31

[Boris Zbarsky [:bzbarsky]]

The hope is to get this landed ASAP now that fx6 has branched.  Just blocked on the reviews...

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

I'm very well aware that I should review some patches here.
I'll try to do that during this weekend.

Comment 33

[Mounir Lamouri (:mounir)]

Comment on attachment 532662 [details] [diff] [review]
part 12.  Add dom::Element::UpdateState and use it in various places where elements update their own state.

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

::: content/html/content/src/nsHTMLFormElement.cpp
@@ +382,5 @@
>  {
>    if (aName == nsGkAtoms::novalidate && aNameSpaceID == kNameSpaceID_None) {
>      // Update all form elements states because they might be [no longer]
>      // affected by :-moz-ui-valid or :-moz-ui-invalid.
> +    nsAutoScriptBlocker scriptBlocker;

Do we still need this?

@@ +1721,2 @@
>  
> +        nsAutoScriptBlocker scriptBlocker;

Why is this still needed?

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +634,5 @@
> +
> +  // Set up our default state.  By default we're enabled (since we're
> +  // a control type that can be disabled but not actually disabled
> +  // right now), optional, and valid.  We are NOT readwrite by default
> +  // until someone calls UpdateEditableState on us, apparently!  Also

Actually, we should make sure that an element that isn't inside a document never gets the readwrite state, see bug 620766. So, in the long term, that's the behavior we want I think.

@@ -905,5 @@
> -                NS_EVENT_STATE_MOZ_UI_VALID |
> -                NS_EVENT_STATE_MOZ_UI_INVALID |
> -                NS_EVENT_STATE_INDETERMINATE |
> -                NS_EVENT_STATE_MOZ_PLACEHOLDER |
> -                NS_EVENT_STATE_MOZ_SUBMITINVALID;

Lovely :)

@@ +3792,5 @@
>    UpdateTypeMismatchValidityState();
>    UpdatePatternMismatchValidityState();
>  
> +  if (validBefore != IsValid()) {
> +    UpdateState(aNotify);

Couldn't this be simplified by removing the check? UpdateState() is already checking that don't call ContentStateChanged() for nothing, isn't it?

::: content/html/content/src/nsHTMLSelectElement.cpp
@@ +2275,5 @@
>    PRBool previousSelectionChangedValue = mSelectionHasChanged;
>    mSelectionHasChanged = aValue;
>  
> +  if (mSelectionHasChanged != previousSelectionChangedValue) {
> +    UpdateState(aNotify);

Like for <input>, can't we just assume that UpdateState() isn't going to do unnecessary calls if no states have actually changed?

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +324,5 @@
> +
> +  // Set up our default state.  By default we're enabled (since we're
> +  // a control type that can be disabled but not actually disabled
> +  // right now), optional, and valid.  We are NOT readwrite by default
> +  // until someone calls UpdateEditableState on us, apparently!  Also

Comment made for <input> applies here too.

@@ +1196,4 @@
>      }
>  
> +    if (aName == nsGkAtoms::readonly) {
> +      UpdateEditableState(aNotify);

Makes me realize we should make <input> and <textarea> read-only when they are disabled, shouldn't we?

@@ +1453,5 @@
>    UpdateValueMissingValidityState();
>  
> +  if (validBefore != IsValid() ||
> +      (HasAttr(kNameSpaceID_None, nsGkAtoms::placeholder)
> +       && !nsContentUtils::IsFocusedContent((nsIContent*)(this)))) {

Should we remove this check?

Comment 34

[Boris Zbarsky [:bzbarsky]]

> Do we still need this?

I think so; otherwise script could posibly run after any of the state changes we're doing.

> Why is this still needed?

Same reason.

> Actually, we should make sure that an element that isn't inside a document
> never gets the readwrite state, see bug 620766.

Great; we can update this code in that bug.  ;)  For now I was aiming to preserve existing behavior, period.

> Couldn't this be simplified by removing the check? 

It could; the cost would be an extra IntrinsicState() call.  Let me know if you'd prefer that.

> Like for <input>, can't we just assume that UpdateState() isn't going to do
> unnecessary calls if no states have actually changed?

Yes.  This, again, seemed like a simple way of optimizing out an IntrinsicState call.

> Makes me realize we should make <input> and <textarea> read-only when they
> are disabled, shouldn't we?

Are they read-write in that situation right now?  Or are they neither?

> Should we remove this check?

Which one?  The validBefore check or the rest?

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532638 [details] [diff] [review]
part 1.  Move IntrinsicState from nsIContent to Element.

>diff --git a/content/base/public/nsIContent.h b/content/base/public/nsIContent.h
>--- a/content/base/public/nsIContent.h
>+++ b/content/base/public/nsIContent.h
Update IID

> MakeContentDescendantsEditable(nsIContent *aContent, nsIDocument *aDocument)
> {
>-  nsEventStates stateBefore = aContent->IntrinsicState();
>-
>-  aContent->UpdateEditableState();
>-
>-  if (aDocument && stateBefore != aContent->IntrinsicState()) {
>-    aDocument->ContentStateChanged(aContent,
>+  if (!aContent->IsElement()) {
>+    aContent->UpdateEditableState();
>+    return;
>+  }
>+
>+  Element *element = aContent->AsElement();
>+  nsEventStates stateBefore = element->IntrinsicState();
>+
>+  element->UpdateEditableState();
>+
>+  if (aDocument && stateBefore != element->IntrinsicState()) {
>+    aDocument->ContentStateChanged(element,
>                                    NS_EVENT_STATE_MOZ_READONLY |
>                                    NS_EVENT_STATE_MOZ_READWRITE);
Please add some comment before if (!aContent->IsElement()) about which
states are updated in which cases.


>-  PRUint32 i, n = aContent->GetChildCount();
>-  for (i = 0; i < n; ++i) {
>-    nsIContent *child = aContent->GetChildAt(i);
>+  for (nsIContent *child = aContent->GetFirstChild();
>+       child;
>+       child = child->GetNextSibling()) {
Nothing to do with this bug, but ok.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 532640 [details] [diff] [review]
part 3.  Store hover and active state directly on elements.

Maybe rename NotifyAncestors to ChangeAncestors or something else that 
indicates it does more than notify?  Also, the aStateAdded parameter
sounds like it should be a state rather than a boolean.  Maybe
aIsAddition?

>+          NotifyAncestors(notifyContent1, commonAncestor, aState, PR_TRUE);
>+        } else {
>+          NotifyAncestors(notifyContent1, commonAncestor, aState,
>+                          content1StateSet);
>         } 

You don't actually need an else here; you could merge this into:

>         }
>         NotifyAncestors(notifyContent1, commonAncestor, aState,
>                         content1StateSet);

r=dbaron

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 532641 [details] [diff] [review]
part 4.  Store urltarget and dragover state directly on elements.

Same comment about the aStateAdded param as in patch 3, and same
comment about not needing the else.

r=dbaron

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Update IID

Done.

> Please add some comment before if (!aContent->IsElement()) about which
> states are updated in which cases.

Done.

> Maybe rename NotifyAncestors to ChangeAncestors or something else that 
> indicates it does more than notify?

Renamed to UpdateAncestorState.

> You don't actually need an else here; you could merge this into:

Good catch.  Done.

Same for part 4.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532642 [details] [diff] [review]
part 5.  Store focus and focusring state directly on elements.


> private:
>-  // Need to allow the ESM to set our state
>+  // Need to allow the ESM, nsGlobalWindow, and the focus manager to
>+  // set our state
>   friend class ::nsEventStateManager;
>+  friend class ::nsGlobalWindow;
>+  friend class ::nsFocusManager;
Hmm, why is :: needed before class name?

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532649 [details] [diff] [review]
part 7.  Make IntrinsicState protected.

Btw, could you file a followup to rename nsEventState to ContentState, or should it be ElementState.

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532648 [details] [diff] [review]
part 6.  Get rid of nsEventStateManager::GetContentState.

> inDOMUtils::GetContentState(nsIDOMElement *aElement, nsEventStates::InternalType* aState)
> {
>   *aState = 0;
> 
>   NS_ENSURE_ARG_POINTER(aElement);
> 
>-  nsRefPtr<nsEventStateManager> esm = inLayoutUtils::GetEventStateManagerFor(aElement);
>-  if (esm) {
>-    nsCOMPtr<nsIContent> content;
>-    content = do_QueryInterface(aElement);
>+  nsCOMPtr<nsIContent> content;
>+  content = do_QueryInterface(aElement);

Not really your code, but maybe
  *aState = 0;
  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
  NS_ENSURE_ARG_POINTER(content);

  // NOTE: if this method is removed,
  // please remove GetInternalValue from nsEventStates
  *aState = content->AsElement()->State().GetInternalValue();
  return NS_OK;


>--- a/layout/inspector/tests/test_bug462789.html
>+++ b/layout/inspector/tests/test_bug462789.html
>@@ -66,19 +66,19 @@ function do_test() {
>     var res = utils.getBindingURLs(docElement);
>     is(res instanceof Components.interfaces["nsIArray"], true, "getBindingURLs result type");
>     is(res.length, 0, "getBindingURLs array length");
>   }
>   catch(e) { ok(false, "got an unexpected exception:" + e); }
> 
>   try {
>     utils.getContentState(docElement);
>-    ok(false, "expected an exception"); 
>+    ok(true, "Should not throw"); 
>   }
>-  catch(e) { is(e.result, ERROR_FAILURE, "got the expected exception"); }
>+  catch(e) { ok(false, "Got an exception: " + e); }
Yeah, I think this change to behavior makes sense.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532653 [details] [diff] [review]
part 8.  Create an explicit API to request link state updates.   is a bit of a hack to make sure that we update link state sometime when doing style resolution, pending a better setup in followup bugs for determining when to resolve the link URI


>+  /**
>+   * Request an update of the link state for this element.  This will
>+   * make sure that if the element is a link at all then either
>+   * NS_EVENT_STATE_VISITED or NS_EVENT_STATE_UNVISITED is set in
>+   * mState, and a history lookup kicked off if needed to find out
>+   * whether the link is really visited.  This method will NOT send any
>+   * state change notifications, if you want them to happen for this
>+   * call, you need to handle them yourself.
Nit, maybe '.', not ',' after 'notifications'




>+void
>+nsHTMLAnchorElement::RequestLinkStateUpdate()
>+{
>+  UpdateLinkState(Link::LinkState());
>+}
>+
> already_AddRefed<nsIURI>

>+void
>+nsHTMLAreaElement::RequestLinkStateUpdate()
>+{
>+  UpdateLinkState(Link::LinkState());
>+}
>+

>+void
>+nsHTMLLinkElement::RequestLinkStateUpdate()
>+{
>+  UpdateLinkState(Link::LinkState());
>+}

>+void
>+nsSVGAElement::RequestLinkStateUpdate()
>+{
>+  UpdateLinkState(Link::LinkState());
>+}
I wish we could have some common base class for all the link elements, but that is tricky.


> nsCSSRuleProcessor::GetContentState(Element* aElement)
> {
>+  aElement->RequestLinkStateUpdate();

> nsCSSRuleProcessor::IsLink(Element* aElement)
> {
>+  aElement->RequestLinkStateUpdate();
These need some good comments. They do indeed look rather hacky, so I'd like the
comment to include even the followup bug number.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532655 [details] [diff] [review]
part 9.  Store an Element pointer in Link.

Huh, the Link setup looks strange.
But this patch makes it a least a tiny bit better.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 532662 [details] [diff] [review]
part 12.  Add dom::Element::UpdateState and use it in various places where elements update their own state.

>   nsEventStates State() const {
>-    return IntrinsicState() | mState;
>+    return mState;
>   }
Could you add some comment that mState is enough since it should
contain also IntrinsicState (based on Element::UpdateState).


>+     * @param aNotify if false, only need to update the state of our element.
In general, I wish there was some comment somewhere, when it is ok to just update the
state without notifying.



>+nsGenericHTMLFormElement::UpdateEditableFormControlState(PRBool aNotify)
> {
>   // nsCSSFrameConstructor::MaybeConstructLazily is based on the logic of this
>   // function, so should be kept in sync with that.
> 
>   ContentEditableTristate value = GetContentEditableValue();
>   if (value != eInherit) {
>     SetEditableFlag(!!value);
>-
>+    UpdateState(aNotify);
>     return;
>   }
> 
>   nsIContent *parent = GetParent();
> 
>   if (parent && parent->HasFlag(NODE_IS_EDITABLE)) {
>     SetEditableFlag(PR_TRUE);
>+    UpdateState(aNotify);
>     return;
>   }
> 
>   if (!IsTextControl(PR_FALSE)) {
>     SetEditableFlag(PR_FALSE);
>+    UpdateState(aNotify);
>     return;
>   }
> 
>   // If not contentEditable we still need to check the readonly attribute.
>   PRBool roState;
>   GetBoolAttr(nsGkAtoms::readonly, &roState);
> 
>   SetEditableFlag(!roState);
>+  UpdateState(aNotify);
> }
UpdateState makes things a bit fragile here. Could you not have SetEditableFlag to call it
automatically? Or some helper method which calls SetEditableFlag and UpdateState?


>   if (HasAttr(kNameSpaceID_XLink, nsGkAtoms::href)) {
>+    // XXXbz it would be nice if we could just have ClearBrokenState
>+    // update our state and do it fast...
File a followup and add the bug number here?


>+    bool IsReadWriteTextElement() const {
Nit, { should be in the next line


/me likes.


r=me

Comment 45

[Mounir Lamouri (:mounir)]

(In reply to comment #34)
> > Makes me realize we should make <input> and <textarea> read-only when they
> > are disabled, shouldn't we?
> 
> Are they read-write in that situation right now?  Or are they neither?

They are read-write right now. I will check if we have a bug open for that or file one if needed.

Comment 46

[Neil Deakin]

Comment on attachment 532642 [details] [diff] [review]
part 5.  Store focus and focusring state directly on elements.

>-static void
>-NotifyFocusStateChange(nsIContent* aContent, nsPIDOMWindow* aWindow)
>+/* static */
>+void
>+nsFocusManager::NotifyFocusStateChange(nsIContent* aContent,
>+                                       PRBool aWindowShouldShowFocusRing,
>+                                       PRBool aGettingFocus)

Would be nice to move this method so it wasn't sandwiched between the nsIFocusManager interface methods.


> {
>-  nsIDocument *doc = aContent->GetCurrentDoc();
>-  nsAutoScriptBlocker scriptBlocker;


You remove the scriptBlocker here but not the usage in nsGlobalWindow::SetKeyboardIndicators.

Comment 47

[Boris Zbarsky [:bzbarsky]]

> Hmm, why is :: needed before class name?

Because otherwise the compiler thinks I'm forward-declaring mozilla::dom::nsFocusManager and friending it at the same time, and then the compile fails later because nsFocusManager is not a friend.

> Btw, could you file a followup to rename nsEventState to ContentState

Filed bug 660954.

> Not really your code, but maybe

Done.

> Nit, maybe '.', not ',' after 'notifications'

Done.

> I wish we could have some common base class for all the link elements, but
> that is tricky.

Indeed.

> These need some good comments. 

Done, and added bug 660959 to the commit message too.

> Could you add some comment that mState is enough 

Done.

> In general, I wish there was some comment somewhere, when it is ok to just
> update the state without notifying.

Added.  Short story: when the element can't have a frame no matter what its style is.

> Could you not have SetEditableFlag to call it automatically?

No, because SetEditableFlag is inlined on nsINode.

> Or some helper method which calls SetEditableFlag and UpdateState?

Done, nsGenericHTMLElement::DoSetEditableFlag.

> File a followup and add the bug number here?

Done, bug 660963.

> Nit, { should be in the next line

Done.

> Would be nice to move this method so it wasn't sandwiched

Done.

> You remove the scriptBlocker here but not the usage in
> nsGlobalWindow::SetKeyboardIndicators.

Good catch.  Removed that one too.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/projects/cedar/rev/bed26fbfb169
  http://hg.mozilla.org/projects/cedar/rev/49dde1235442
  http://hg.mozilla.org/projects/cedar/rev/56396cb2536a
  http://hg.mozilla.org/projects/cedar/rev/18c34313f5f4
  http://hg.mozilla.org/projects/cedar/rev/3a4140939fc4
  http://hg.mozilla.org/projects/cedar/rev/693f73d9ee75
  http://hg.mozilla.org/projects/cedar/rev/a847b71c7685
  http://hg.mozilla.org/projects/cedar/rev/a9578a576fa6
  http://hg.mozilla.org/projects/cedar/rev/678ae0f96b82
  http://hg.mozilla.org/projects/cedar/rev/1d8707e111c0
  http://hg.mozilla.org/projects/cedar/rev/fa98ebd237d0
  http://hg.mozilla.org/projects/cedar/rev/5ad2fcf8d9ed

and looking green.

Comment 49

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/bed26fbfb169
http://hg.mozilla.org/mozilla-central/rev/49dde1235442
http://hg.mozilla.org/mozilla-central/rev/56396cb2536a
http://hg.mozilla.org/mozilla-central/rev/18c34313f5f4
http://hg.mozilla.org/mozilla-central/rev/3a4140939fc4
http://hg.mozilla.org/mozilla-central/rev/693f73d9ee75
http://hg.mozilla.org/mozilla-central/rev/a847b71c7685
http://hg.mozilla.org/mozilla-central/rev/a9578a576fa6
http://hg.mozilla.org/mozilla-central/rev/678ae0f96b82
http://hg.mozilla.org/mozilla-central/rev/1d8707e111c0
http://hg.mozilla.org/mozilla-central/rev/fa98ebd237d0
http://hg.mozilla.org/mozilla-central/rev/5ad2fcf8d9ed

Comment 50

[Boris Zbarsky [:bzbarsky]]

Talos says:
  Talos Improvement! Ts decrease 1.19% on Nokia n900 mobile
  Talos Improvement! Txul decrease 3.01% on MacOSX 10.5.8 Firefox
  Talos Improvement! Txul decrease 4.33% on Nokia n900 mobile
  Talos Improvement! Txul decrease 3.55% on Linux x64 Firefox

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Interesting that this affected to Txul so much.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Well, it sped up style resolution some...  and we have a _lot_ of rules applying to XUL elements.

Comment 53

[Ioana (away)]

test_bug598833-1.html passed on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420285&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420362&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420482&tree=Mozilla-Beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420477&tree=Mozilla-Beta

layout/reftests/css-default/submit-button/default-multiple-submit-1.html,
layout/reftests/css-default/submit-button/default-multiple-submit-2.html and
layout/reftests/css-default/submit-button/default-multiple-submit-ref.html
passed on all OSs:

Comment 54

[Ioana (away)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=6420133&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420565&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420372&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420330&full=1