492250 - URLs in Page Info Media tab should be LTR

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[(no longer active)]

The URLs in the Media tab of the Page Info dialog should appear LTR even if the UI is RTL.

This is similar to bug 478430.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 376617 [details] [diff] [review]
Patch (v1)

>diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js

>+  getCellProperties: function(row, column, prop) {
>+    if (column.element.getAttribute("id") == "image-address")

Can you not check column.element.id instead?

(Unrelated to this patch, but it looks like columnids and colcount are unused in this file?)

Comment 3

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #2)
> (From update of attachment 376617 [details] [diff] [review])
> >diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js
> 
> >+  getCellProperties: function(row, column, prop) {
> >+    if (column.element.getAttribute("id") == "image-address")
> 
> Can you not check column.element.id instead?

Yes.  Done in the new revision.  Also moved the check to the getCellProperties which actually gets used... :/

> (Unrelated to this patch, but it looks like columnids and colcount are unused
> in this file?)

Yeah, I think so.  I'll file a new bug on this.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 377641 [details] [diff] [review]
Patch (v2)

Well that's confusing. Can you remove the unused getCellProperties, and cache both atoms used by the real one ("ltr" and "broken")?

Comment 5

[(no longer active)]

Attachment: Patch (v3)

(In reply to comment #4)
> (From update of attachment 377641 [details] [diff] [review])
> Well that's confusing. Can you remove the unused getCellProperties, and cache
> both atoms used by the real one ("ltr" and "broken")?

The other getCellProperties gets used by the meta tags tree.

I added the code to cache "broke" in this version of the patch.

Comment 6

[(no longer active)]

(In reply to comment #3)
> > (Unrelated to this patch, but it looks like columnids and colcount are unused
> > in this file?)
> 
> Yeah, I think so.  I'll file a new bug on this.

Bug 493225 filed.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 377644 [details] [diff] [review]
Patch (v3)

>diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js

>-    props.AppendElement(aserv.getAtom("broken"));
>+    props.AppendElement(aserv.getAtom(this._brokenAtom));

You're calling getAtom and passing an atom? Even if that works, it's not optimal!

I'd be a lot more comfortable reviewing patches if this functionality had some tests. Several of the patches for this bug appear to have been untested (original one didn't touch the right function, this one potentially breaks broken-styling, etc.)

Comment 8

[(no longer active)]

Attachment: Patch (v3.1)

(In reply to comment #7)
> (From update of attachment 377644 [details] [diff] [review])
> >diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js
> 
> >-    props.AppendElement(aserv.getAtom("broken"));
> >+    props.AppendElement(aserv.getAtom(this._brokenAtom));
> 
> You're calling getAtom and passing an atom? Even if that works, it's not
> optimal!

Fixed.

> I'd be a lot more comfortable reviewing patches if this functionality had some
> tests. Several of the patches for this bug appear to have been untested
> (original one didn't touch the right function, this one potentially breaks
> broken-styling, etc.)

You're right, but I'm not sure how I can test this.  As a demo of how this patch looks, I took a screenshot of the Page Info window for this URL: data:text/html,<img src="http://www.google.com/"><img src="http://www.google.com/logo.png">

http://grab.by/4x6o

The platform level functionality landed in bug 478377 is tested in the reftest suite.  Maybe we should have Litmus tests here?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 446990 [details] [diff] [review]
Patch (v3.1)

aserv is now unused, r=me with it removed.

Comment 10

[(no longer active)]

Attachment: Patch to land

Comment 11

[Tanner M. Young [:tmyoung]]

(In reply to comment #8)
> The platform level functionality landed in bug 478377 is tested in the reftest
> suite.  Maybe we should have Litmus tests here?

What exactly do you want the litmus test to do (since I don't entirely understand what this patch does)?

Comment 12

[(no longer active)]

(In reply to comment #11)
> (In reply to comment #8)
> > The platform level functionality landed in bug 478377 is tested in the reftest
> > suite.  Maybe we should have Litmus tests here?
> 
> What exactly do you want the litmus test to do (since I don't entirely
> understand what this patch does)?

I don't particularly think that a Litmus test is necessary here, but I'll leave the final decision to Gavin.

Comment 13

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7edcbc1fb0ee

Comment 14

[:Gavin Sharp [email: [e-mail]]]

let's not worry about it.