1009935 - Implement the @autocomplete attribute for values other than off/on

Status: VERIFIED FIXED

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

Description

[Matthew N. [:MattN]]

Password manager, form manager and requestAutocomplete would benefit from implementing the new values for the autocomplete attribute. Currently, each of these areas are doing their own getAttribute calls but duplicating the tokenizing logic in each component is error prone and doesn't allow proper feature detection by websites.

Comment 1

[Matthew N. [:MattN]]

I think that the spec is a bit more complex than is needed initially with the various sectioning tokens so I will send an email to whatwg about trimming the spec a bit.

Comment 2

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 3

[Matthew N. [:MattN]]

Attachment: WIP 1 - Excludes section-*

Olli, I think I've covered most cases in the spec. Could you take a look at the approach before I finish populating the EnumTables with the token values we will do anything useful with. e.g. I don't think our autocomplete dropdown or requestAutocomplete will do anything useful with the "impp" hint so I think it makes sense to allow for feature detection by returning "" from the element's property when it's used.

This doesn't implement section-* as I'm hoping to get it dropped from the spec.
Discussion: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2014-May/296824.html
WHATWG Bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25735

Comment 4

[Matthew N. [:MattN]]

Attachment: v.1 - Tests

Bug fixes noticed during testing and one bit of cleanup.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8425355 [details] [diff] [review]
v.1 - Tests

Oops, those were the tests I was going to upload next.

Comment 6

[Matthew N. [:MattN]]

Attachment: WIP 1.1 - Excludes section-*

Comment 7

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

Huh, the API is crazy. Going through the spec first.

Comment 8

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

Comment on attachment 8425355 [details] [diff] [review]
v.1 - Tests

>+ // Two tokens
>+ ["on off", ""],
>+ ["off on", ""],
>+ ["username tel", ""],
>+ ["tel username ", ""],
>+ [" username tel ", ""],
>+ ["shipping tel", "shipping tel"],
>+ ["shipPING tel", "shipping tel"],
I'd add still tests for different ordering.
Like, 
"tel shipping".
If I interpret the spec
(which isn't too clear about this autocomplete stuff) correctly,
that should lead to "".

We need tests for different form types. Not only type="text".

Comment 9

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

Comment on attachment 8425361 [details] [diff] [review]
WIP 1.1 - Excludes section-*

>+enum AutocompleteFieldName {
{ goes to its own line.

>+  AutocompleteFieldName_OFF,
I'd prefer eAutocompleteFieldName_OFF
and same with others
>+static const nsAttrValue::EnumTable kAutocompleteFieldNameTable[] = {
>+  { "off", AutocompleteFieldName_OFF },
>+  { "on", AutocompleteFieldName_ON },
>+  { "name", AutocompleteFieldName_NAME },
>+  // TODO...
>+  { "username", AutocompleteFieldName_USERNAME },
>+  { "new-password", AutocompleteFieldName_NEW_PASSWORD },
>+  { "current-password", AutocompleteFieldName_CURRENT_PASSWORD },
>+  // ...
>+  { 0 }
>+};

If we add support for more autocomplete types, perhaps we could add them all.
Just not have real backend for all the type.

>+    // Only allow on/off if experimental @autocomplete values aren't enabled.
>+    if (!Preferences::GetBool("dom.forms.autocomplete.experimental")) {
>+      return false;
>+    }
Add some AddIntVarCache so that we don't need to do slow-ish hashtable lookup all the time


>+NS_IMETHODIMP
>+HTMLInputElement::GetAutocomplete(nsAString& aValue)
>+{
>+  nsAutoString attributeVal;
>+  GetAttr(kNameSpaceID_None, nsGkAtoms::autocomplete, attributeVal);
>+  nsContentUtils::ParseAutocompleteAttribute(attributeVal, aValue);
>+  return NS_OK;
We need to be able to cache the value somehow. Otherwise this might be too slow.
Could we perhaps use atomarray for the AttrValue and then add a flag to HTMLInputElement to
tell whether that list if valid. If not, return empty string here, otherwise
stringify that atomlist. Even that might be a bit slow, but at least we wouldn't parse stuff
several times.

Comment 10

[Matthew N. [:MattN]]

Attachment: v.2 - Part 2 - Tests including @type

(In reply to Olli Pettay [:smaug] from comment #8)
> I'd add still tests for different ordering.
> Like, 
> "tel shipping".
> If I interpret the spec
> (which isn't too clear about this autocomplete stuff) correctly,
> that should lead to "".

Yes, that's correct. I've added a 3 new test cases.

> We need tests for different form types. Not only type="text".

Okay, I thought about it initially but since the code wasn't doing anything with @type, I didn't bother. I've now included the types that all non-multiline autocomplete values apply to.

Comment 11

[Matthew N. [:MattN]]

Attachment: WIP 2 - Added pref caching, prevent re-parsing with every get

Addressed comment 9 feedback.

I'm not sure if how I addressed the performance issue is what you had in mind. I'm not a huge fan of the different code paths taken to generate the string but it's not too bad.

Olli, could you treat this like a review since I think it's in good shape and I  just need to populate the various enums and EnumTables next. Note that I'm not an expert in C++ or the platform APIs so please keep that in mind.

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

I had some test failures locally but I'm not sure how/if they're related yet. I'll look into them tomorrow if they're also on our infra.

Thanks

Comment 12

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

Just a tiny bit red tryserver ;)

Comment 13

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

Comment on attachment 8426835 [details] [diff] [review]
v.2 - Part 2 - Tests including @type

I guess this is enough for now.

Comment 14

[(no longer active)]

I believe this deserves an intent to ship thread?

Comment 15

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

Comment on attachment 8426848 [details] [diff] [review]
WIP 2 - Added pref caching, prevent re-parsing with every get

>+++ b/content/base/public/nsContentUtils.h
>@@ -18,16 +18,17 @@
> #endif
> 
> #include "js/TypeDecls.h"
> #include "js/Value.h"
> #include "js/RootingAPI.h"
> #include "mozilla/EventForwards.h"
> #include "mozilla/GuardObjects.h"
> #include "mozilla/TimeStamp.h"
>+#include "nsAttrValueInlines.h"

Hmm, you need this for enums, right? Could you move those enums to .cpp.

>+nsContentUtils::AutocompleteAttrState
>+nsContentUtils::ParseAutocompleteAttribute(const nsAttrValue* aAttr,
>+                                           nsAString& aResult)
>+{
>+  AutocompleteAttrState state;
>+  bool valid = InternalParseAutocompleteAttribute(aAttr, aResult);
>+  if (valid) {
>+    ASCIIToLower(aResult);
>+    state = eAutocompleteAttrState_Valid;
>+  } else {
>+    aResult = EmptyString();
>+    state = eAutocompleteAttrState_Invalid;
>+  }
>+  return state;
>+}

This would become a bit simpler if InternalParseAutocompleteAttribute returned AutocompleteAttrState, 



So, f+, but would like to see a new patch (and tryserver push which is more green.)

Comment 16

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

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)
> I believe this deserves an intent to ship thread?

Definitely. The spec is rather odd, but apparently this is shipping at least in some other
browsers, so perhaps it can't be fixed anymore. (and in this particular case I don't care too much
about the spec being bad - finding a good API for this stuff might just take too much time.)

Comment 17

[Matthew N. [:MattN]]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #14)
> > I believe this deserves an intent to ship thread?
> 
> Definitely. The spec is rather odd, but apparently this is shipping at least
> in some other
> browsers, so perhaps it can't be fixed anymore. (and in this particular case
> I don't care too much
> about the spec being bad - finding a good API for this stuff might just take
> too much time.)

I thought about it last week but I'm not sure it does since Brian already sent one for requestAutocomplete which requires this and is arguably the same feature.

Comment 18

[(no longer active)]

(In reply to comment #17)
> (In reply to Olli Pettay [:smaug] from comment #16)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #14)
> > > I believe this deserves an intent to ship thread?
> > 
> > Definitely. The spec is rather odd, but apparently this is shipping at least
> > in some other
> > browsers, so perhaps it can't be fixed anymore. (and in this particular case
> > I don't care too much
> > about the spec being bad - finding a good API for this stuff might just take
> > too much time.)
> 
> I thought about it last week but I'm not sure it does since Brian already sent
> one for requestAutocomplete which requires this and is arguably the same
> feature.

There is no harm in sending one for this specific feature.  :-)

Comment 19

[Matthew N. [:MattN]]

Attachment: v.1 Part 1 - No section-*; with initial tokens to support

Comment 15 addressed.

This patch also adds the list of tokens we initially think we'll support. Should I leave the commented lines for the related fields which we aren't supporting? Should I include commented out lines for the other unrelated fields that we don't intend to support (ever or initially)?

There were no test failures on a try push earlier today.
New v.1 try push that should be all green: https://tbpl.mozilla.org/?tree=Try&rev=85b847c4aa44

Ehsan, also note that this is behind a pref so it would be an intent to implement, not ship. We don't have a timeline for shipping yet. I'll send the intent shortly.

Thanks

Comment 20

[Matthew N. [:MattN]]

Intent to implement: https://groups.google.com/d/topic/mozilla.dev.platform/TJ8ePk0T__k/discussion

Comment 21

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

Comment on attachment 8427525 [details] [diff] [review]
v.1 Part 1 - No section-*; with initial tokens to support

>+  enum AutocompleteAttrState
>+  {
>+    eAutocompleteAttrState_Unknown = 1,
>+    eAutocompleteAttrState_Invalid,
>+    eAutocompleteAttrState_Valid,
>+  };
This enum could use MOZ_ENUM_TYPE(uint8_t) I think


>+  /**
>+   * Parses the value of the autocomplete attribute into aResult, ensuring it's
>+   * compoased
composed


>+enum AutocompleteFieldName
>+{
>+  eAutocompleteFieldName_OFF,
>+  eAutocompleteFieldName_ON,
>+
>+  // Name types
>+  eAutocompleteFieldName_NAME,
>+  //eAutocompleteFieldName_HONORIFIC_PREFIX,
>+  eAutocompleteFieldName_GIVEN_NAME,
>+  eAutocompleteFieldName_ADDITIONAL_NAME,
>+  eAutocompleteFieldName_FAMILY_NAME,
>+  //eAutocompleteFieldName_HONORIFIC_SUFFIX,
>+  //eAutocompleteFieldName_NICKNAME,
>+  //eAutocompleteFieldName_ORGANIZATION_TITLE,
>+
>+  // Login types
>+  eAutocompleteFieldName_USERNAME,
>+  eAutocompleteFieldName_NEW_PASSWORD,
>+  eAutocompleteFieldName_CURRENT_PASSWORD,
>+
>+  // Address types
>+  eAutocompleteFieldName_ORGANIZATION,
>+  eAutocompleteFieldName_STREET_ADDRESS,
>+  eAutocompleteFieldName_ADDRESS_LINE1,
>+  eAutocompleteFieldName_ADDRESS_LINE2,
>+  eAutocompleteFieldName_ADDRESS_LINE3,
>+  eAutocompleteFieldName_ADDRESS_LEVEL1,
>+  eAutocompleteFieldName_ADDRESS_LEVEL2,
>+  eAutocompleteFieldName_ADDRESS_LEVEL3,
>+  eAutocompleteFieldName_ADDRESS_LEVEL4,
>+  eAutocompleteFieldName_COUNTRY,
>+  eAutocompleteFieldName_COUNTRY_NAME,
>+  eAutocompleteFieldName_POSTAL_CODE,
>+
>+  // Phone number types
>+  eAutocompleteFieldName_TEL,
>+  eAutocompleteFieldName_TEL_COUNTRY_CODE,
>+  eAutocompleteFieldName_TEL_NATIONAL,
>+  eAutocompleteFieldName_TEL_AREA_CODE,
>+  eAutocompleteFieldName_TEL_LOCAL,
>+  eAutocompleteFieldName_TEL_LOCAL_PREFIX,
>+  eAutocompleteFieldName_TEL_LOCAL_SUFFIX,
>+  eAutocompleteFieldName_TEL_EXTENSION,
>+
>+  // Email
>+  eAutocompleteFieldName_EMAIL,
>+};
>+
>+enum AutocompleteCategory
>+{
>+  eAutocompleteCategory_NORMAL,
>+  eAutocompleteCategory_CONTACT,
>+};
>+
>+enum AutocompleteFieldHint
>+{
>+  eAutocompleteFieldHint_SHIPPING,
>+  eAutocompleteFieldHint_BILLING,
>+
>+  // Contact hints
>+  eAutocompleteFieldHint_HOME,
>+  eAutocompleteFieldHint_WORK,
>+  eAutocompleteFieldHint_MOBILE,
>+  eAutocompleteFieldHint_FAX,
>+  //eAutocompleteFieldHint_PAGER,
Why not pager?


>+};
>+
>+static const nsAttrValue::EnumTable kAutocompleteFieldNameTable[] = {
>+  { "off", eAutocompleteFieldName_OFF },
>+  { "on", eAutocompleteFieldName_ON },
>+
>+  { "name", eAutocompleteFieldName_NAME },
>+  { "given-name", eAutocompleteFieldName_GIVEN_NAME },
>+  { "additional-name", eAutocompleteFieldName_ADDITIONAL_NAME },
>+  { "family-name", eAutocompleteFieldName_FAMILY_NAME },
>+
>+  { "username", eAutocompleteFieldName_USERNAME },
>+  { "new-password", eAutocompleteFieldName_NEW_PASSWORD },
>+  { "current-password", eAutocompleteFieldName_CURRENT_PASSWORD },
>+
>+  { "organization", eAutocompleteFieldName_ORGANIZATION },
>+  { "street-address", eAutocompleteFieldName_STREET_ADDRESS },
>+  { "address-line1", eAutocompleteFieldName_ADDRESS_LINE1 },
>+  { "address-line2", eAutocompleteFieldName_ADDRESS_LINE2 },
>+  { "address-line3", eAutocompleteFieldName_ADDRESS_LINE3 },
>+  { "address-level1", eAutocompleteFieldName_ADDRESS_LEVEL1 },
>+  { "address-level2", eAutocompleteFieldName_ADDRESS_LEVEL2 },
>+  { "address-level3", eAutocompleteFieldName_ADDRESS_LEVEL3 },
>+  { "address-level4", eAutocompleteFieldName_ADDRESS_LEVEL4 },
>+  { "country", eAutocompleteFieldName_COUNTRY },
>+  { "country-name", eAutocompleteFieldName_COUNTRY_NAME },
>+  { "postal-code", eAutocompleteFieldName_POSTAL_CODE },
>+  { 0 }
>+};
>+
>+static const nsAttrValue::EnumTable kAutocompleteContactFieldNameTable[] = {
>+  { "tel", eAutocompleteFieldName_TEL },
>+  { "tel-country-code", eAutocompleteFieldName_TEL_COUNTRY_CODE },
>+  { "tel-national", eAutocompleteFieldName_TEL_NATIONAL },
>+  { "tel-area-code", eAutocompleteFieldName_TEL_AREA_CODE },
>+  { "tel-local", eAutocompleteFieldName_TEL_LOCAL },
>+  { "tel-local-prefix", eAutocompleteFieldName_TEL_LOCAL_PREFIX },
>+  { "tel-local-suffix", eAutocompleteFieldName_TEL_LOCAL_SUFFIX },
>+  { "tel-extension", eAutocompleteFieldName_TEL_EXTENSION },
>+
>+  { "email", eAutocompleteFieldName_EMAIL },
>+  { 0 }
>+};
>+
>+static const nsAttrValue::EnumTable kAutocompleteContactFieldHintTable[] = {
>+  { "home", eAutocompleteFieldHint_HOME },
>+  { "work", eAutocompleteFieldHint_WORK },
>+  { "mobile", eAutocompleteFieldHint_MOBILE },
>+  { "fax", eAutocompleteFieldHint_FAX },
At least add commented out entry here for pager.


>+  { 0 }
>+};
>+
>+static const nsAttrValue::EnumTable kAutocompleteFieldHintTable[] = {
>+  { "shipping", eAutocompleteFieldHint_SHIPPING },
>+  { "billing", eAutocompleteFieldHint_BILLING },
>+  { 0 }
>+};


I think it would be easier to review if we had entries for all the values from the spec, and then
explicitly comment out the ones we can't support first.
(but it is still a bit unclear to me how this all is supposed to work if some values are supported and some aren't)


(You could have implemented the enums and EnumTables using some macro magic. 
 In that case the values would need to be listed only once.
 EventNameList.h is a complicated example of such.
 But I'm not sure whether it would make the code significantly simpler in this case.
)



Should ParseAutocompleteAttribute be actually
SerializeAutocompleteAttribute


>+  uint32_t index = numTokens - 1;
>+  nsString tokenString;
>+  nsIAtom* lastAtom = aAttrVal->AtomAt(index);
>+  lastAtom->ToString(tokenString);
You could use nsDependentAtomString



>@@ -1469,16 +1456,18 @@ HTMLInputElement::AfterSetAttr(int32_t a
>         nsAutoString value;
>         GetValueInternal(value);
>         nsNumberControlFrame* numberControlFrame =
>           do_QueryFrame(GetPrimaryFrame());
>         if (numberControlFrame) {
>           numberControlFrame->SetValueOfAnonTextControl(value);
>         }
>       }
>+    } else if (aName == nsGkAtoms::autocomplete) {
>+      mAutocompleteAttrState = nsContentUtils::eAutocompleteAttrState_Unknown;
>     }
This could use a comment that we're clearing the cached state.


>+NS_IMETHODIMP
>+HTMLInputElement::GetAutocomplete(nsAString& aValue)
>+{
>+  const nsAttrValue* attributeVal = GetParsedAttr(nsGkAtoms::autocomplete);
>+  if (!attributeVal ||
>+      mAutocompleteAttrState == nsContentUtils::eAutocompleteAttrState_Invalid) {
>+    aValue.Assign(EmptyString());
>+    return NS_OK;
>+  }
>+  if (mAutocompleteAttrState == nsContentUtils::eAutocompleteAttrState_Valid) {
>+    aValue.Truncate(0);
Just do aValue.Truncate(); at the beginning of the method.
Then no need to assign EmptyString() nor Truncate(0);


>+    for (uint32_t i = 0; i < attributeVal->GetAtomCount(); i++) {
I don't quite trust compilers to cache GetAtomCount() here, so could you assign it to some
variable and then use that in the for loop.


>+      if (i != 0) {
>+        aValue.Append(' ');
>+      }
>+      nsString tokenString;
>+      nsIAtom* atom = attributeVal->AtomAt(i);
>+      atom->ToString(tokenString);
>+      aValue.Append(tokenString);
No need for tokenString.

aValue.Append(nsDependentAtomString(attributeVal->AtomAt(i)));

Comment 22

[Matthew N. [:MattN]]

Attachment: v.2 Part 1 - No section-*; with initial tokens to support

(In reply to Olli Pettay [:smaug] from comment #21)
> >+  // Contact hints
> >+  eAutocompleteFieldHint_HOME,
> >+  eAutocompleteFieldHint_WORK,
> >+  eAutocompleteFieldHint_MOBILE,
> >+  eAutocompleteFieldHint_FAX,
> >+  //eAutocompleteFieldHint_PAGER,
> Why not pager?

Mostly since Blink doesn't implement it either (probably because it's an outdated piece of technology).

> I think it would be easier to review if we had entries for all the values
> from the spec, and then
> explicitly comment out the ones we can't support first.

OK, I went both ways on including all of them (especially ones I think we'll never implement) but I've done that now. Do you want them all commented out in EnumTables too?

> (but it is still a bit unclear to me how this all is supposed to work if
> some values are supported and some aren't)

We have the option of dispatching "disabled" in rAc if we don't support a field name and a site could do feature detection on their own in advance by checking if we return an empty string for input.autocomplete when the author specified a value.

> (You could have implemented the enums and EnumTables using some macro magic. 
>  In that case the values would need to be listed only once.
>  EventNameList.h is a complicated example of such.
>  But I'm not sure whether it would make the code significantly simpler in
> this case.
> )

Yeah, I thought about using macros but I wasn't familiar with existing patterns for this in other DOM code and wasn't sure it was worth it.

> Should ParseAutocompleteAttribute be actually
> SerializeAutocompleteAttribute

That makes sense as it is now, but later we may want it to persist more information in member variables to make it easier for higher-level code to get the various parts of the attribute without re-splitting the tokens and/or duplicating parts of the processing model. Thoughts?

Comment 23

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

(In reply to Matthew N. [:MattN] from comment #22)
> That makes sense as it is now, but later we may want it to persist more
> information in member variables to make it easier for higher-level code to
> get the various parts of the attribute without re-splitting the tokens
> and/or duplicating parts of the processing model. Thoughts?
Well, in that case the method would start to do something quite different, and the name could change.
but for now, Serialize...

Comment 24

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

Comment on attachment 8434566 [details] [diff] [review]
v.2 Part 1 - No section-*; with initial tokens to support

>+  eAutocompleteFieldName_ADDRESS_LEVEL1,
>+  eAutocompleteFieldName_ADDRESS_LEVEL2,
>+  eAutocompleteFieldName_ADDRESS_LEVEL3,
>+  eAutocompleteFieldName_ADDRESS_LEVEL4,
For some reason these are in different order in the spec.
>+  // Additional field types
>+  /*
>+  eAutocompleteFieldName_LANGUAGE,
>+  eAutocompleteFieldName_BDAY,
>+  eAutocompleteFieldName_BDAY_DAY,
>+  eAutocompleteFieldName_BDAY_MONTH,
>+  eAutocompleteFieldName_YEAR,
This should be BDAY_YEAR

>+enum AutocompleteFieldHint
>+{
>+  eAutocompleteFieldHint_SHIPPING,
>+  eAutocompleteFieldHint_BILLING,
>+
>+  // Contact hints
>+  eAutocompleteFieldHint_HOME,
>+  eAutocompleteFieldHint_WORK,
>+  eAutocompleteFieldHint_MOBILE,
>+  eAutocompleteFieldHint_FAX,
>+  //eAutocompleteFieldHint_PAGER,
>+};
Since you have two different nsAttrValue::EnumTables for this stuff, could you split the
enum too.


Could you add some dummy _last entry to all the enums, and then do some MOZ_ASSERT that
..._last == ArrayLength(kAutocomplete...Table);
That would enforce that all the entries in the enums are handled in EnumTables


>+nsContentUtils::AutocompleteAttrState
>+nsContentUtils::ParseAutocompleteAttribute(const nsAttrValue* aAttr,
>+                                           nsAString& aResult)
>+{
>+  AutocompleteAttrState state = InternalParseAutocompleteAttribute(aAttr, aResult);
>+  if (state == eAutocompleteAttrState_Valid) {
>+    ASCIIToLower(aResult);
>+  } else {
>+    aResult.Assign(EmptyString());
aResult.Truncate();


>+  if (category == eAutocompleteCategory_CONTACT) {
>+    nsAttrValue contactFieldHint;
>+    result = contactFieldHint.ParseEnumValue(tokenString, kAutocompleteContactFieldHintTable, false);
>+    if (result) {
>+      aResult.Insert(' ', 0);
>+      nsString contactFieldHintString;
nsAutoString

Comment 25

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

Comment on attachment 8434566 [details] [diff] [review]
v.2 Part 1 - No section-*; with initial tokens to support

>+  index -= 1;
--index;

Comment 26

[Matthew N. [:MattN]]

Attachment: v.3 Part 1 - No section-*; with initial tokens to support. r=smaug

I addressed all comments.

(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8434566 [details] [diff] [review]
> v.2 Part 1 - No section-*; with initial tokens to support
> 
> >+  eAutocompleteFieldName_ADDRESS_LEVEL1,
> >+  eAutocompleteFieldName_ADDRESS_LEVEL2,
> >+  eAutocompleteFieldName_ADDRESS_LEVEL3,
> >+  eAutocompleteFieldName_ADDRESS_LEVEL4,
> For some reason these are in different order in the spec.

That's because I added them before they got put in the spec yesterday. I only noticed the different order after the spec commit.

> >+enum AutocompleteFieldHint
> >+{
> >+  eAutocompleteFieldHint_SHIPPING,
> >+  eAutocompleteFieldHint_BILLING,
> >+
> >+  // Contact hints
> >+  eAutocompleteFieldHint_HOME,
> >+  eAutocompleteFieldHint_WORK,
> >+  eAutocompleteFieldHint_MOBILE,
> >+  eAutocompleteFieldHint_FAX,
> >+  //eAutocompleteFieldHint_PAGER,
> >+};
> Since you have two different nsAttrValue::EnumTables for this stuff, could
> you split the
> enum too.

OK, they have the same name for these tokens in the spec so that's why I made them as one enum. Only where they are allowed is what differs. I changed this now.

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

Comment 27

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21cf9d1e6f48
https://hg.mozilla.org/integration/mozilla-inbound/rev/da60b8bc18a3

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/21cf9d1e6f48
https://hg.mozilla.org/mozilla-central/rev/da60b8bc18a3