196380 - Image location copying code should use nsIImageLoadingContent

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

This came up in bug 182700 where code was added to be able to copy the image
location of other images than HTML IMG elements. That code currently has
specific code for different types of images when it should use
nsIImageLoadingContent once it's available.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

taking...

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

OK. so, to fix this, I'll add an readonly attribute nsIURI currentURI; attribute
to nsIImageLoadingContent, with the following behaviour:
o) If mCurrentRequest != null, returns its uri
o) otherwise, it returns the last uri for which ImageURIChanged was called

afaict that will always result in "the url of the image that's currently displayed".

bz, did I understand this code right?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sounds like you did, yes.

I assume the "mCurrentRequest == null" case is meant to cover blocked images? 
That is, I'd sort of like us to not be keeping the URI around unless
mCurrentRequest is in fact null....

Note that I think some existing consumers of nsIImageLoadingContent may benefit
from this change.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #3)
> I assume the "mCurrentRequest == null" case is meant to cover blocked images? 

that too; the actual use case I was thinking of is when LoadImage fails (i.e.
when AsyncOpen fails)

> That is, I'd sort of like us to not be keeping the URI around unless
> mCurrentRequest is in fact null....

sure, seems doable

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

So....

consider:
data:text/html,<img src="foo">
(that exact uri. not a document with same contents from file:)

What do you expect when you do "copy link location"
o) as a user?
o) as a developer who knows you can't get an nsIURI for the image uri? (about:
has no relative links)

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm... also, what about malformed uris?

maybe nsImageLoadingContent should store the uri as string, and give it back as
such...

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v0

this is what I have so far; but it can't copy urls for which StringToURI fails.


It also moves nsCopySupport from layout to content - I feel it belongs there
better; it also doesn't really need any layout stuff. I'll attach a patch that
shows the changes I made to it in a second.

And I eliminated the CopyImageLocation and DoCopyImageContents functions from
nsIPresShell.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: nsCopySupport changes

Comment 9

[Boris Zbarsky [:bzbarsky]]

> hmm... also, what about malformed uris?

We won't be showing the image for those, now would we?  In my opinion, the
isImage bool in the context menu code should just be false for broken images....

Comment 10

[[e-mail]]

So how should the context menu go about detecting an image?

Comment 11

[Boris Zbarsky [:bzbarsky]]

QI to nsIImageLoadingContent and see whether it has a non-failed request.

We can expose functionality on nsIImageLoadingContent as needed to ease this, I
think.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: xpfe patch

ok, together with this patch, I'm happy with the other patch. it only shows
image-related context menu items if the image has an nsIURI-compatible uri.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148986 [details] [diff] [review]
patch v0

>Index: accessible/src/base/Makefile.in
>+			imglib2 \

Fix the indent?

>Index: content/base/public/nsCopySupport.h
>+#ifndef nsCopySupport_h_
>+#define nsCopySupport_h_

I think our convention is two underscores after the 'h' there.

>Index: content/base/public/nsIImageLoadingContent.idl

>+   * Otherwise, returns the URI that was last attempted to load,
>+   * or null if no url was attempted to load yet.

  "Otherwise, returns the last URI that this content tried to load, or
   null if there haven't been any such attempts."

>Index: content/base/src/nsCopySupport.cpp

Note bug 197375.  With this patch and that one, nsCopySupport may well not
depend on layout at all...

I assume you're getting someone to move the file in CVS so as to preserve the
history?  Please coordinate with smontagu so he checks into the right one,
unless he checks in before you start doing your stuff.

>Index: content/base/src/nsImageLoadingContent.cpp

>+#ifdef DEBUG_chb
>+#endif

/* DEBUG_chb */, right? 

>+nsImageLoadingContent::GetCurrentURI(nsIURI** aURI) {

Put the '{' on the next line, per style in the file.

>+  // If this succeeded and we affected the current request, then we don't need
>+  // the URI anymore; release it to save memory.

I think "to save memory" can be dropped here... ;)

>+  if ((&req == &mCurrentRequest) && NS_SUCCEEDED(rv)) {
>+    mCurrentURI = nsnull;
>+  }

Just testing |if (mCurrentRequest)| should be enough, no?  If we got this far
_and_ we have an mCurrentRequest, we shouldn't need the mCurrentURI.

>Index: modules/libpref/src/init/all.js

This seems to have diluted the purity of the patch unnecessarily.  Didn't you
land this part already?  ;)

r+sr=bzbarsky with above comments addressed.

Comment 14

[[e-mail]]

Comment on attachment 149081 [details] [diff] [review]
xpfe patch

It's a pity that a bug to clean up nsContextMenu.js wouldn't make a good first
bug :-(

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, and please do make sure you file bugs on tbird and firefox to fix their
context menu code up too...

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 149081 [details] [diff] [review]
xpfe patch

I'll file those bugs once I check this in

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149081 [details] [diff] [review]
xpfe patch

sr=bzbarsky; file followups on making imagemaps work too?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #17)
> (From update of attachment 149081 [details] [diff] [review])
> sr=bzbarsky; file followups on making imagemaps work too?

I filed that followup bug already ;) (bug 60561 - which is a duplicate of bug 33583)

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: final patch

leaf copied the file in cvs

This is a final patch, applies against trunk, and addresses bz's comments

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

filed Bug 245661 and Bug 245660 for thunderbird and firefox

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 22

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

This looks like it cause a ~0.5% pageload time regression on btek.