Closed
Bug 162845
Opened 23 years ago
Closed 23 years ago
PPEmbed needs native impl of download progress UI
Categories
(Core Graveyard :: Embedding: Mac, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.2alpha
People
(Reporter: ccarlen, Assigned: ccarlen)
Details
Attachments
(4 files, 2 obsolete files)
127.20 KB,
patch
|
mikepinkerton
:
review+
sfraser_bugs
:
superreview+
chofmann
:
approval+
|
Details | Diff | Splinter Review |
15.84 KB,
application/octet-stream
|
Details | |
81.59 KB,
image/jpeg
|
Details | |
18.60 KB,
text/plain
|
Details |
Currently, it's using the same XUL impl as mozilla.
Assignee | ||
Comment 1•23 years ago
|
||
Taking.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2alpha
Just want to make sure:
not only will you be replacing the xul with native, but you will also be using the new
native progresslistener for webpersist operations like "Save page as..." and "Save link
as...".
Assignee | ||
Comment 3•23 years ago
|
||
Simon did CHeaderSniffer.cpp/.h, UCustomNavServicesDialogs.cpp/.h and most of
the changes to CBrowserShell.cpp.
Assignee | ||
Comment 4•23 years ago
|
||
Assignee | ||
Comment 5•23 years ago
|
||
Looking for review. I'm now going to make a better icon for the close pane
control, but that won't affect the diffs.
Comment 6•23 years ago
|
||
+ if (LEventDispatcher::GetCurrentEventDispatcher()) { // Can this ever
be NULL?
+ EventRecord theEvent;
+ while (::WaitNextEvent(updateMask | activMask, &theEvent, 0, nil))
+
LEventDispatcher::GetCurrentEventDispatcher()->DispatchEvent(theEvent);
+ }
this seems a tad skanky. where did this code come from? was it always there?
Why are the files called UDownload.cpp/.h when the class in them is CDownload?
You do this in a couple of places.
+ // An download is beginning. Initialize the UI for this download.
Nit. Should read "A download".
+ Boolean cmdHandled = false;
+
+ if (!cmdHandled)
+ cmdHandled = LWindow::ObeyCommand(inCommand, ioParam);
+
+ return cmdHandled;
How about just:
return LWindow::ObeyCommand(inCommand, ioParam);
and if you're going to do that, why have this routine at all?
+ const float kOneThousand24 = 1024.0;
oh now this is just plain silly ;) ;)
there's a lot here, it's hard to wrap my head around all of the relationships.
how well have you tested things like closing the dialog while it's in the middle
of downloading, cancelling one d/l while others are still going on, etc? It took
a while to get all the ownership bugs weeded out of the chimera implementation,
especially with things like quitting the app while downloading.
tenative r=pink.
Assignee | ||
Comment 7•23 years ago
|
||
> this seems a tad skanky. where did this code come from? was it always there?
I had to add it for this. The problem is that the save dialog goes away, which
generates an activate event for the window behind it. Then, the download
progress dialog is opened before returning to the main event loop and that
generates another set of activate events. Processing the pending
activate/updates immediately after dismissing the dialog fixes this.
> Why are the files called UDownload.cpp/.h when the class in them is CDownload?
There's also CHelperAppLauncherDialog in that file. I like to begin file names
of files implementing more than 1 object with "U" for "Unit." Sorry, my MacApp
roots are showing.
> and if you're going to do that, why have this routine at all?
Whoops - yeah that method needs to go away.
> + const float kOneThousand24 = 1024.0;
oh now this is just plain silly ;) ;)
Do you know that if you type 1024.0 n times in a function, that there are not n
8-byte globals in your PEF? Since I wasn't completely sure, I used 1 const float.
> how well have you tested things like closing the dialog while it's in the
> middle of downloading, cancelling one d/l while others are still going on
Quite well. I put code in to alert the user when closing the window on an
in-progress download that it will cancel the download. Same thing on quitting
the app.
Assignee | ||
Comment 8•23 years ago
|
||
Cleaned up a few things Pink pointed out, especially the empty ObeyCommand()
Made a 32-bit icon for the "close pane" control and some nescesary changes to
CIconServicesIcon.
Got rid of CMultiDownloadProgress::HandleAppleEvent. This is a big comment at
the top of UDownloadDisplay explaining it.
Attachment #96756 -
Attachment is obsolete: true
Assignee | ||
Comment 9•23 years ago
|
||
Attachment #96757 -
Attachment is obsolete: true
Assignee | ||
Comment 10•23 years ago
|
||
Also added in the updated patch: getting the font styles for the static text
items in the dialog with GetThemeFont(). Here's a screenshot of it.
Comment 11•23 years ago
|
||
just a ui suggestion:
i'd rather see the gray (x) like used in projectBuilder and the search text
field in jaguar rather than the red circle to close a particular panel.
Assignee | ||
Comment 12•23 years ago
|
||
A diff between the two patches for the sake of Pink who reviewed the first one.
It's a little hard to understand, but hopefully easier than reviewing the new
patch from scratch
Comment 13•23 years ago
|
||
Comment on attachment 96906 [details] [diff] [review]
updated patch
r=pink
Attachment #96906 -
Flags: review+
Comment 14•23 years ago
|
||
Comment on attachment 96906 [details] [diff] [review]
updated patch
sr=sfraser
Attachment #96906 -
Flags: superreview+
Comment 15•23 years ago
|
||
Comment on attachment 96906 [details] [diff] [review]
updated patch
a=chofmann for 1.0.2
Attachment #96906 -
Flags: approval+
Assignee | ||
Comment 16•23 years ago
|
||
Checked into branch. Leaving open until it's in the trunk.
Keywords: fixed1.0.2
Updated•23 years ago
|
QA Contact: mdunn → depstein
Assignee | ||
Comment 17•23 years ago
|
||
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 18•23 years ago
|
||
Please verify the bug. Once verified, change the keyword fixed1.0.2 to
verified1.0.2
Updated•23 years ago
|
QA Contact: depstein → dsirnapalli
Comment 19•23 years ago
|
||
-- Marking as verified.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.2 → verified1.0.2
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•