22.81 KB, patch
|Details | Diff | Splinter Review|
26.81 KB, patch
|Details | Diff | Splinter Review|
the HTTP protocol handler should listen to changes in useragent prefs.
Target Milestone: --- → Future
http bugs to "Networking::HTTP"
Assignee: gagan → darin
Component: Networking → Networking: HTTP
Target Milestone: Future → M19
Added Topembed keyword. Mozilla not paying attention to the changes in useragent preferences. This can cause wrong user agent on the Embedded app startup (on new user profiles only).
Target Milestone: mozilla1.0 → mozilla0.9.5
why is this necessary? we already listen for general.useragent.override. why would different profiles need to modify portions of the default useragent string?
There aren't any comments on this bug since the 17th of Sept. Can QA regess against the Netscape commercial builds and determine if this is still a valid bug? If so, and we can get fixes/reviews in the next day or two, please mark as nsbranch+ which will get this on the PDT radar. Else, mark is as nsbranch-. Also, can someone comment in the bug how serious you think this is? PDT is only accepting "stop ship" bugs such as data loss and loss of major functionality.
override was an afterthought. the need here is in individual component modification. this bug was created long before the override stuff was put in, and IMO, is still valid. caching the whole string and parsing in/out components will lead to folks banking on the format of the string (which as history tells us, isn't good). the format should be left up to impl to deal w/, and consumers should just have to worry about their component.
ok, but nsIHttpHandler allows any component the freedom to change the UA parts. why do we want to be able to program the UA parts via prefs?
Because nsIPrefService is an API which is exposed to embeddors and nsIHttpHandler isn't. Unless we want to freeze nsIHttpHandler and expose it but I bet we don't.
ok.. that definitely makes sense. then does this need to be nsbranch?
nsbranch and topembed are synonymous right now I think. topembed means drive it into the 0.9.4 branch. I'll PDT+ it.
so i understand this to mean that the http handler should simply listen to pref changes on anything matching "general.useragent." ... correct? attaching a patch to make it so.
Yeah - just use the nsIPrefBranchInternal::AddObserver() stuff instead of the old C callbacks. I bet you knew that but just making sure ;-)
yeah, i'm going to try to completely cleanup the pref code in http.
jud: can you review this?
cc'ing bnesse so he can see this too. Brian, why does Darin need the "internal" interface here? IIRC, it's because we don't expose observation publically; right? Looks good to me. My nested #define macro know-how is rusty though, your #define PREF_CHANGED() macro refers to "pref" but I don't see that declared anywhere. Does your UA_PREF() "_pref" get translated to "pref" when nested? If the answer is yes to my above question, r=valeski.
Jud, yes that's correct. The Observation stuff was placed into the internal interface because there were questions as to whether or not the Observer interface was going to be exposed to embedding IIRC. The patch looks great. Thanks darin for wacking one off my list of interfaces that need to be cleaned up.
jud: pref is the argument to PrefsChanged... _pref is the argument to the UA_PREF macro... they are not the same thing.
thanks for clarification.
Comment on attachment 50859 [details] [diff] [review] v1.0 patch, cleans up and completes http pref handling sr=mscott
Attachment #50859 - Flags: superreview+
pulling branch '+', because this isn't needed for eMojo. but leaving topembed because we'll need this on 0.9.4 after 6.2 is done.
removing PDT - The PDT
-> mozilla0.9.4 for checkin on the branch
Target Milestone: mozilla0.9.5 → mozilla0.9.4
This checkin caused thi tinderbox leak stats to increase: see bug 102332.
has this hit the 0.9.4 branch yet?
pls check this into the branch - PDT+, once you have good confidence.
Whiteboard: fixed-on-trunk → fixed-on-trunk [PDT+]
Comment on attachment 50859 [details] [diff] [review] v1.0 patch, cleans up and completes http pref handling firstname.lastname@example.org Looks good to me. The only thing that i noticed was that the UA_PREF.misc was checked for twice :-) you might want to remove the extra one... -- rick
Comment on attachment 52810 [details] [diff] [review] patch for the 0.9.4 branch, including leak fixes Looks good. r=bnesse.
Attachment #52810 - Flags: review+
Comment on attachment 52810 [details] [diff] [review] patch for the 0.9.4 branch, including leak fixes email@example.com this patch makes my eyes cross :-)
Attachment #52810 - Flags: superreview+
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
* useragent string for version (misc) appears in console window. * useragent string for locale didn't appear in the console, but Darren explained this will appear in the user agent. * Accept-Language entries are listed and in correct order. * Accept-Charset entry is listed, also 0.66 "other" entry. * User-Agent string incls app name, version, platform, OS: "Mozilla", "5.0", "Windows", "WinNT4.0"
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.