Closed
Bug 52510
Opened 24 years ago
Closed 23 years ago
HTTP handler should register as a listener for user agent prefs changes.
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla0.9.4
People
(Reporter: jud, Assigned: darin.moz)
References
Details
(Keywords: topembed, Whiteboard: fixed-on-trunk [PDT+])
Attachments
(2 files)
(deleted),
patch
|
jud
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bnesse
:
review+
rpotts
:
superreview+
|
Details | Diff | Splinter Review |
the HTTP protocol handler should listen to changes in useragent prefs.
http bugs to "Networking::HTTP"
Assignee: gagan → darin
Component: Networking → Networking: HTTP
Target Milestone: Future → M19
Assignee | ||
Updated•24 years ago
|
Target Milestone: --- → Future
Assignee | ||
Updated•24 years ago
|
Target Milestone: Future → mozilla1.0
Comment 3•23 years ago
|
||
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).
Keywords: topembed
Assignee | ||
Updated•23 years ago
|
Priority: P3 → P1
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•23 years ago
|
||
why is this necessary? we already listen for general.useragent.override. why
would different profiles need to modify portions of the default useragent string?
Comment 6•23 years ago
|
||
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.
Reporter | ||
Comment 7•23 years ago
|
||
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.
Assignee | ||
Comment 8•23 years ago
|
||
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?
Comment 9•23 years ago
|
||
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.
Assignee | ||
Comment 10•23 years ago
|
||
ok.. that definitely makes sense. then does this need to be nsbranch?
Reporter | ||
Comment 11•23 years ago
|
||
nsbranch and topembed are synonymous right now I think. topembed means drive it
into the 0.9.4 branch. I'll PDT+ it.
Whiteboard: PDT+
Assignee | ||
Comment 13•23 years ago
|
||
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.
Comment 14•23 years ago
|
||
Yeah - just use the nsIPrefBranchInternal::AddObserver() stuff instead of the
old C callbacks. I bet you knew that but just making sure ;-)
Assignee | ||
Comment 15•23 years ago
|
||
yeah, i'm going to try to completely cleanup the pref code in http.
Assignee | ||
Comment 16•23 years ago
|
||
Assignee | ||
Comment 17•23 years ago
|
||
jud: can you review this?
Reporter | ||
Comment 18•23 years ago
|
||
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.
Comment 19•23 years ago
|
||
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.
Reporter | ||
Updated•23 years ago
|
Attachment #50859 -
Flags: review+
Assignee | ||
Comment 20•23 years ago
|
||
jud: pref is the argument to PrefsChanged... _pref is the argument to the UA_PREF
macro... they are not the same thing.
Reporter | ||
Comment 21•23 years ago
|
||
thanks for clarification.
Comment 22•23 years ago
|
||
Comment on attachment 50859 [details] [diff] [review]
v1.0 patch, cleans up and completes http pref handling
sr=mscott
Attachment #50859 -
Flags: superreview+
Reporter | ||
Comment 23•23 years ago
|
||
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.
Assignee | ||
Comment 26•23 years ago
|
||
-> mozilla0.9.4 for checkin on the branch
Target Milestone: mozilla0.9.5 → mozilla0.9.4
Comment 27•23 years ago
|
||
This checkin caused thi tinderbox leak stats to increase: see bug 102332.
Reporter | ||
Comment 28•23 years ago
|
||
has this hit the 0.9.4 branch yet?
Assignee | ||
Comment 29•23 years ago
|
||
nope
Comment 30•23 years ago
|
||
pls check this into the branch - PDT+, once you have good confidence.
Whiteboard: fixed-on-trunk → fixed-on-trunk [PDT+]
Comment 31•23 years ago
|
||
Comment on attachment 50859 [details] [diff] [review]
v1.0 patch, cleans up and completes http pref handling
r=rpotts@netscape.com
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
Assignee | ||
Comment 32•23 years ago
|
||
Comment 33•23 years ago
|
||
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 34•23 years ago
|
||
Comment on attachment 52810 [details] [diff] [review]
patch for the 0.9.4 branch, including leak fixes
sr=rpotts@netscape.com
this patch makes my eyes cross :-)
Attachment #52810 -
Flags: superreview+
Assignee | ||
Comment 35•23 years ago
|
||
fixed-on-0.9.4-branch
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 36•23 years ago
|
||
* 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.
Description
•