Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
911303002:
Fix text wrapping problem in the download_item_controller on Yosemite. (Closed)
DescriptionFix text wrapping problem in the download_item_controller on Yosemite.
The code in [DownloadItemController showDangerousWarning:] calls
[GTMUILocalizerAndLayoutTweaker sizeToFitFixedHeightTextField:minWidth:]
to wrap the warning string within the DownloadItemController's
dangerousDownloadLabel_ textField. Within the nib this textField is 22
points tall and holds two lines of text. Under Yosemite, however, the
font metrics have changed and the two lines are now 24 points tall.
This cl insets the dangerousDownloadLabel_'s frame height by -2 points
when running on Yosemite. dangerousDownloadLabel_ is not as tall as its
superview, so the new frame still fits comfortably within the superview's
frame.
BUG=454782
TEST=Follow instructions in bug 454782 and confirm that text is wrapping
properly on Yosemite.
Committed: https://crrev.com/4ba4f927840199e4b84a530b43ad2de7402e157c
Cr-Commit-Position: refs/heads/master@{#318927}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Swapped in IsOSYosemiteOrLater() for IsOSYosemite() #Patch Set 3 : Changed how compute textfield resize delta #
Total comments: 6
Patch Set 4 : Simplified code to compute required field height #Patch Set 5 : Resize download item textfield in xib rather than in code #Messages
Total messages: 35 (5 generated)
shrike@chromium.org changed reviewers: + thakis@chromium.org
This is the simplest approach I could think of to address the issue, but please let me know if you have a cleaner/better alternative you'd like me to explore.
thakis@chromium.org changed reviewers: + asanka@chromium.org
https://codereview.chromium.org/911303002/diff/1/chrome/browser/ui/cocoa/down... File chrome/browser/ui/cocoa/download/download_item_controller.mm (right): https://codereview.chromium.org/911303002/diff/1/chrome/browser/ui/cocoa/down... chrome/browser/ui/cocoa/download/download_item_controller.mm:137: if (base::mac::IsOSYosemite()) { IsOSYosemiteOrLater() probably?
PTAL https://codereview.chromium.org/911303002/diff/1/chrome/browser/ui/cocoa/down... File chrome/browser/ui/cocoa/download/download_item_controller.mm (right): https://codereview.chromium.org/911303002/diff/1/chrome/browser/ui/cocoa/down... chrome/browser/ui/cocoa/download/download_item_controller.mm:137: if (base::mac::IsOSYosemite()) { On 2015/02/11 03:45:53, Nico wrote: > IsOSYosemiteOrLater() probably? Done.
lgtm, I can't think of anything better either. Can't think of a good way to test for this either.
Hello asanka, thakis added you as a reviewer - do you have any comments on this cl? Thank you.
I think you can land this. I added asanka more as an fyi for him.
On 2015/02/20 at 21:06:38, thakis wrote: > I think you can land this. I added asanka more as an fyi for him. Whoops sorry about the delay. I'm a little worried about the sensitivity here. Is it possible we could run into similar issues due to localization? What happens if the user changes the system font? Is that possible?
New patchsets have been uploaded after l-g-t-m from thakis@chromium.org
On 2015/02/20 21:19:16, asanka wrote: > On 2015/02/20 at 21:06:38, thakis wrote: > > I think you can land this. I added asanka more as an fyi for him. > > Whoops sorry about the delay. > > I'm a little worried about the sensitivity here. Is it possible we could run > into similar issues due to localization? What happens if the user changes the > system font? Is that possible? I don't believe Apple provides a way for users to change the system font, but I believe it's possible to do so with hacks. Also, it seems like the Japanese and Korean fonts need a little more height than a Roman font at the same point size. In any case I rewrote this code so that on Yosemite and higher it uses the textfield to compute the height required to display two lines of text, and then adjusts the textfield's frame accordingly. I believe that should address your concerns? thakis, PTAL
Assuming thakis is okay with the general direction of the CL. https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/download/download_item_controller.mm (right): https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/download/download_item_controller.mm:140: // after the computation (except for, of course, its height). Comment is out of date? https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/download/download_item_controller.mm:143: NSString* placeholderText = [dangerousDownloadLabel_ stringValue]; Let's move this height adjustment logic into -showDangerousWarning: https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/download/download_item_controller.mm:145: [dangerousDownloadLabel_ setStringValue:@"Two lines\nof text"]; Once this is moved to -showDangerousWarning:, we could use something like [NSString stringWithFormat:@"%@\n%@", dangerousWarning, dangerousWarning] to get the height from the actual text we want to display, in case that makes a difference.
I'll hand this off to asanka. Computing is fine with me too. (I'd do the computation unconditionally, not just on yosemite though, then it gets more testing and whatnot)
PTAL https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/download/download_item_controller.mm (right): https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/download/download_item_controller.mm:140: // after the computation (except for, of course, its height). On 2015/02/21 02:14:53, asanka wrote: > Comment is out of date? It's not, but I rewrote it in the new cl. https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/download/download_item_controller.mm:145: [dangerousDownloadLabel_ setStringValue:@"Two lines\nof text"]; On 2015/02/21 02:14:54, asanka wrote: > Once this is moved to -showDangerousWarning:, we could use something like > [NSString stringWithFormat:@"%@\n%@", dangerousWarning, dangerousWarning] to get > the height from the actual text we want to display, in case that makes a > difference. The idea behind this change is just to quickly fix up the textfield's height on nib load so that the textfield works as expected thereafter. The textfield already contains placeholder text that wraps to two lines in the nib, but I couldn't think of a way to get sizeToFit to use that placeholder text to compute the two line height (the Appkit wanted to lay it out on a single line). That's why I replaced it with a temp string that forced wrapping (ugly, but as a I said, I couldn't come up with another way to do it). I have since reworked this code to use the existing placeholder string to generate the two-line height. I think it's cleaner/simpler to make this little adjustment once at nib load time rather than every time showDangerousWarning: gets called. Additionally, this adjustment is really independent of whatever string comes into showDangerousWarning:. Finally, per thakis's recommendation, I have removed the yosemite check.
https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/download/download_item_controller.mm (right): https://codereview.chromium.org/911303002/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/download/download_item_controller.mm:145: [dangerousDownloadLabel_ setStringValue:@"Two lines\nof text"]; On 2015/02/23 at 20:12:14, shrike wrote: > On 2015/02/21 02:14:54, asanka wrote: > > Once this is moved to -showDangerousWarning:, we could use something like > > [NSString stringWithFormat:@"%@\n%@", dangerousWarning, dangerousWarning] to get > > the height from the actual text we want to display, in case that makes a > > difference. > > The idea behind this change is just to quickly fix up the textfield's height on nib load so that the textfield works as expected thereafter. The textfield already contains placeholder text that wraps to two lines in the nib, but I couldn't think of a way to get sizeToFit to use that placeholder text to compute the two line height (the Appkit wanted to lay it out on a single line). That's why I replaced it with a temp string that forced wrapping (ugly, but as a I said, I couldn't come up with another way to do it). > > I have since reworked this code to use the existing placeholder string to generate the two-line height. I think it's cleaner/simpler to make this little adjustment once at nib load time rather than every time showDangerousWarning: gets called. Additionally, this adjustment is really independent of whatever string comes into showDangerousWarning:. showDangerousWarning is called at most once per download. It's a constraint of the downloads system that a download only transitions once from "safe" to "dangerous." Why do you think this adjustment is going to be independent of the string that should be displayed? Given that we aren't making sweeping changes elsewhere in Chrome to accommodate this change in Yosemite, should we consider changing the XIB and avoid runtime changes? > Finally, per thakis's recommendation, I have removed the yosemite check.
On 2015/02/24 03:16:13, asanka wrote: > showDangerousWarning is called at most once per download. It's a constraint of > the downloads system that a download only transitions once from "safe" to > "dangerous." OK. I still think it's cleaner to stick this code in awakeFromNib but I can certainly move it to showDangerousWarning: - if that is still your preference let me know and I will move it there. > Why do you think this adjustment is going to be independent of the string that > should be displayed? There is only enough space in the textfield's superview to display at most two lines of text, so there is no point in trying to resize the textfield to accommodate strings that wrap to three or more lines. If the warning message is short enough to fit on one line, the textfield will already do the right thing without any adjustment (i.e. center the string vertically). So the only thing we're trying to do is make sure the textfield is tall enough to display a string that wraps to two lines. That's my thinking, anyway. Why do you think the adjustment needs to be based on the warning string? > Given that we aren't making sweeping changes elsewhere in Chrome to accommodate > this change in Yosemite, should we consider changing the XIB and avoid runtime > changes? It would be simpler to make the textfield a little bit taller in the xib but modifying the xib can only be done using a particular version of Xcode 4 running on a particular version of OS X. Because of these constraints it's better to put the fix in code.
On 2015/02/24 16:52:07, shrike wrote: > It would be simpler to make the textfield a little bit taller in the xib but > modifying the xib can only be done using a particular version of Xcode 4 running > on a particular version of OS X. Because of these constraints it's better to put > the fix in code. I woke up this morning and realized it shouldn't be much trouble to tweak the xib to fix the textfield. I did something similar with another xib on a previous cl. The steps here are: 1. Use any version of IB to change the textfield's frame, and save in 4.x format 2. git diff the xib to find the one line of xib source that sets textfield's frame to its new value - all other changes are irrelevant 3. git checkout -- to throw away all changes 4. Edit the one line of xib source by hand to set the textfield's new frame If you don't think this is a good way to go let me know, otherwise I will work on making this change and uploading a new cl that only modifies the xib.
On 2015/02/25 at 17:16:36, shrike wrote: > On 2015/02/24 16:52:07, shrike wrote: > > It would be simpler to make the textfield a little bit taller in the xib but > > modifying the xib can only be done using a particular version of Xcode 4 running > > on a particular version of OS X. Because of these constraints it's better to put > > the fix in code. > > I woke up this morning and realized it shouldn't be much trouble to tweak the xib to fix the textfield. I did something similar with another xib on a previous cl. The steps here are: > > 1. Use any version of IB to change the textfield's frame, and save in 4.x format > 2. git diff the xib to find the one line of xib source that sets textfield's frame to its new value - all other changes are irrelevant > 3. git checkout -- to throw away all changes > 4. Edit the one line of xib source by hand to set the textfield's new frame > > If you don't think this is a good way to go let me know, otherwise I will work on making this change and uploading a new cl that only modifies the xib. Yeah. I prefer if we move as much of the layout into the XIB as possible, specially if we are treating the layout issue as something not dependent on the runtime string. However, I'm not qualified to comment on whether the workaround you mention is okay. If it builds and the try jobs succeed and the resulting Chrome binary behaves as expected, then I'm good with it :) Note that you can download binaries out of a try job (https://groups.google.com/a/google.com/forum/#!msg/chrome-team/yk1o_DlYzAA/o7...)
On 2015/02/25 at 17:26:47, asanka wrote: > On 2015/02/25 at 17:16:36, shrike wrote: > > On 2015/02/24 16:52:07, shrike wrote: > > > It would be simpler to make the textfield a little bit taller in the xib but > > > modifying the xib can only be done using a particular version of Xcode 4 running > > > on a particular version of OS X. Because of these constraints it's better to put > > > the fix in code. > > > > I woke up this morning and realized it shouldn't be much trouble to tweak the xib to fix the textfield. I did something similar with another xib on a previous cl. The steps here are: > > > > 1. Use any version of IB to change the textfield's frame, and save in 4.x format > > 2. git diff the xib to find the one line of xib source that sets textfield's frame to its new value - all other changes are irrelevant > > 3. git checkout -- to throw away all changes > > 4. Edit the one line of xib source by hand to set the textfield's new frame > > > > If you don't think this is a good way to go let me know, otherwise I will work on making this change and uploading a new cl that only modifies the xib. > > Yeah. I prefer if we move as much of the layout into the XIB as possible, specially if we are treating the layout issue as something not dependent on the runtime string. > > However, I'm not qualified to comment on whether the workaround you mention is okay. If it builds and the try jobs succeed and the resulting Chrome binary behaves as expected, then I'm good with it :) > Note that you can download binaries out of a try job (https://groups.google.com/a/google.com/forum/#!msg/chrome-team/yk1o_DlYzAA/o7...) thakis: ^^ regarding the XIB workflow workaround.
On 2015/02/25 17:27:27, asanka wrote: > On 2015/02/25 at 17:26:47, asanka wrote: > > On 2015/02/25 at 17:16:36, shrike wrote: > > > On 2015/02/24 16:52:07, shrike wrote: > > > > It would be simpler to make the textfield a little bit taller in the xib > but > > > > modifying the xib can only be done using a particular version of Xcode 4 > running > > > > on a particular version of OS X. Because of these constraints it's better > to put > > > > the fix in code. > > > > > > I woke up this morning and realized it shouldn't be much trouble to tweak > the xib to fix the textfield. I did something similar with another xib on a > previous cl. The steps here are: > > > > > > 1. Use any version of IB to change the textfield's frame, and save in 4.x > format > > > 2. git diff the xib to find the one line of xib source that sets textfield's > frame to its new value - all other changes are irrelevant > > > 3. git checkout -- to throw away all changes > > > 4. Edit the one line of xib source by hand to set the textfield's new frame > > > > > > If you don't think this is a good way to go let me know, otherwise I will > work on making this change and uploading a new cl that only modifies the xib. > > > > Yeah. I prefer if we move as much of the layout into the XIB as possible, > specially if we are treating the layout issue as something not dependent on the > runtime string. > > > > However, I'm not qualified to comment on whether the workaround you mention is > okay. If it builds and the try jobs succeed and the resulting Chrome binary > behaves as expected, then I'm good with it :) > > Note that you can download binaries out of a try job > (https://groups.google.com/a/google.com/forum/#!msg/chrome-team/yk1o_DlYzAA/o7...) > > thakis: ^^ regarding the XIB workflow workaround. works for me too
On 2015/02/25 19:41:55, Nico wrote: > On 2015/02/25 17:27:27, asanka wrote: > okay. If it builds and the try jobs succeed and the resulting Chrome binary > behaves as expected, then I'm good with it :) > Note that you can download binaries out of a try job > > > (https://groups.google.com/a/google.com/forum/#!msg/chrome-team/yk1o_DlYzAA/o7...) I've followed the instructions in this message but have gotten nowhere. My invocation: tools/swarming_client/isolateserver.py download -I isolateserver.appspot.com --isolated=0cd0f4982532c2aacfa28e7fe1b54c15 -t ~/isolate/ results in: 0cd0f4982532c2aacfa28e7fe1b54c15 doesn't seem to be a valid file. Did you intent to pass a valid hash? (And I've played with the server URL and hash specification to see if I wasn't feeding the parameters quite the way it wants them). This is a copy/paste of the hash, so it should be valid. I wondered if perhaps the file had been removed before I had a chance to grab it so I reran the try and attempted to download before the tryserver had finished its work (but after the binary and hash had been generated) and still no luck. This seems like a useful tool, I just don't know how to get it to work. > > > > thakis: ^^ regarding the XIB workflow workaround. > > works for me too The new cl consists just of the modified xib, so PTAL.
On 2015/02/27 at 20:05:11, shrike wrote: > On 2015/02/25 19:41:55, Nico wrote: > > On 2015/02/25 17:27:27, asanka wrote: > > > okay. If it builds and the try jobs succeed and the resulting Chrome binary > > behaves as expected, then I'm good with it :) > > Note that you can download binaries out of a try job > > > > > (https://groups.google.com/a/google.com/forum/#!msg/chrome-team/yk1o_DlYzAA/o7...) > > I've followed the instructions in this message but have gotten nowhere. My invocation: > > tools/swarming_client/isolateserver.py download -I isolateserver.appspot.com --isolated=0cd0f4982532c2aacfa28e7fe1b54c15 -t ~/isolate/ > > results in: > > 0cd0f4982532c2aacfa28e7fe1b54c15 doesn't seem to be a valid file. Did you intent to pass a valid hash? > > (And I've played with the server URL and hash specification to see if I wasn't feeding the parameters quite the way it wants them). This is a copy/paste of the hash, so it should be valid. I wondered if perhaps the file had been removed before I had a chance to grab it so I reran the try and attempted to download before the tryserver had finished its work (but after the binary and hash had been generated) and still no luck. This seems like a useful tool, I just don't know how to get it to work. > > > > > > > thakis: ^^ regarding the XIB workflow workaround. > > > > works for me too > > The new cl consists just of the modified xib, so PTAL. Code LGTM. I don't have a machine that reproduces the issue, so I'm unable to verify the behavior. You can look at json.output of the "isolate tests" step of a builder to figure out the hash to download. For example, the hash map for your mac_chromium_compile_dbg_ng try run is at http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... . This gives you a hash of ce8ba1932b06ea14a740327fa2b2a6eb6c7b2a84 for the browser_tests target.
On 2015/02/27 at 22:50:29, asanka wrote: > On 2015/02/27 at 20:05:11, shrike wrote: > > On 2015/02/25 19:41:55, Nico wrote: > > > On 2015/02/25 17:27:27, asanka wrote: > > > > > okay. If it builds and the try jobs succeed and the resulting Chrome binary > > > behaves as expected, then I'm good with it :) > > > Note that you can download binaries out of a try job > > > > > > > (https://groups.google.com/a/google.com/forum/#!msg/chrome-team/yk1o_DlYzAA/o7...) > > > > I've followed the instructions in this message but have gotten nowhere. My invocation: > > > > tools/swarming_client/isolateserver.py download -I isolateserver.appspot.com --isolated=0cd0f4982532c2aacfa28e7fe1b54c15 -t ~/isolate/ > > > > results in: > > > > 0cd0f4982532c2aacfa28e7fe1b54c15 doesn't seem to be a valid file. Did you intent to pass a valid hash? > > > > (And I've played with the server URL and hash specification to see if I wasn't feeding the parameters quite the way it wants them). This is a copy/paste of the hash, so it should be valid. I wondered if perhaps the file had been removed before I had a chance to grab it so I reran the try and attempted to download before the tryserver had finished its work (but after the binary and hash had been generated) and still no luck. This seems like a useful tool, I just don't know how to get it to work. > > > > > > > > > > thakis: ^^ regarding the XIB workflow workaround. > > > > > > works for me too > > > > The new cl consists just of the modified xib, so PTAL. > > Code LGTM. I don't have a machine that reproduces the issue, so I'm unable to verify the behavior. > > You can look at json.output of the "isolate tests" step of a builder to figure out the hash to download. For example, the hash map for your mac_chromium_compile_dbg_ng try run is at http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... . This gives you a hash of ce8ba1932b06ea14a740327fa2b2a6eb6c7b2a84 for the browser_tests target. Nit about the CL description: We mostly use present tense when describing what the CL does. So something like "Fix text wrapping problem ..." instead of "Fixed..."
On 2015/02/27 22:54:50, asanka wrote: > > You can look at json.output of the "isolate tests" step of a builder to figure > out the hash to download. For example, the hash map for your > mac_chromium_compile_dbg_ng try run is at > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... > . This gives you a hash of ce8ba1932b06ea14a740327fa2b2a6eb6c7b2a84 for the > browser_tests target. Thank you! Next problem is it says it can't authenticate to isolateserver... and to log in using auth.py. I've done that, and doing it again confirms that I'm logged in. I assume I should be logged in using my Chromium account? > Nit about the CL description: We mostly use present tense when describing what > the CL does. So something like "Fix text wrapping problem ..." instead of > "Fixed..." Ack & fixed.
On 2015/02/27 at 23:04:39, shrike wrote: > On 2015/02/27 22:54:50, asanka wrote: > > > You can look at json.output of the "isolate tests" step of a builder to figure > > out the hash to download. For example, the hash map for your > > mac_chromium_compile_dbg_ng try run is at > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_... > > . This gives you a hash of ce8ba1932b06ea14a740327fa2b2a6eb6c7b2a84 for the > > browser_tests target. > > Thank you! Next problem is it says it can't authenticate to isolateserver... and to log in using auth.py. I've done that, and doing it again confirms that I'm logged in. I assume I should be logged in using my Chromium account? Right. You need to authenticate using your @google.com credentials. The tools use your cached token. So you'll have to logout and login again: python auth.py logout -S https://isolateserver.appspot.com python auth.py login -S https://isolateserver.appspot.com And this time, login using your @google.com account. > > Nit about the CL description: We mostly use present tense when describing what > > the CL does. So something like "Fix text wrapping problem ..." instead of > > "Fixed..." > > Ack & fixed.
On 2015/02/27 23:23:24, asanka wrote: > > Thank you! Next problem is it says it can't authenticate to isolateserver... > and to log in using auth.py. I've done that, and doing it again confirms that > I'm logged in. I assume I should be logged in using my Chromium account? > > Right. You need to authenticate using your @google.com credentials. > The tools use your cached token. So you'll have to logout and login again: > > python auth.py logout -S https://isolateserver.appspot.com > python auth.py login -S https://isolateserver.appspot.com > > And this time, login using your @google.com account. Thank you - that worked. Remaining problem is I can't get the browser to flag a download as dangerous. My solution for that while putting the fix together was changing the code in download_target_determiner.cc to flag all files as dangerous but obviously that's not an option in the build downloaded from the server.
On 2015/03/02 17:40:51, shrike wrote: > Thank you - that worked. Remaining problem is I can't get the browser to flag a > download as dangerous. My solution for that while putting the fix together was > changing the code in download_target_determiner.cc to flag all files as > dangerous but obviously that's not an option in the build downloaded from the > server. Despite this issue is it OK to push the change?
On 2015/03/02 21:54:24, shrike wrote: > On 2015/03/02 17:40:51, shrike wrote: > > Thank you - that worked. Remaining problem is I can't get the browser to flag > a > > download as dangerous. My solution for that while putting the fix together was > > changing the code in download_target_determiner.cc to flag all files as > > dangerous but obviously that's not an option in the build downloaded from the > > server. > > Despite this issue is it OK to push the change? If you have the build downloaded, it should be pretty easy to test. The file types for which the dangerous prompt is triggered is in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow... . The warning won't be triggered for explicit downloads ("Save link as.."). So you need to construct something that'll act as an implicit download of a dangerous file. For example: * Search Google for "filetype:swf". SWF is a dangerous file type. * The results page will have lots of links to SWF files marked as [FLASH]. You need to construct implicit download links from these. So pick one, right click and choose inspect element. * The anchor element will look like '<a href="[tracking redirect]" onmousedown="[something]" data-href="[ACTUAL URL]">'. Remove the href and onmousedown attributes, and rename "data-href" to "href". Then add another attribute called "download". * Your link will now look like '<a href="[ACTUAL URL]" download>'. * Click this link. You should see a download warning.
On 2015/03/03 18:05:29, asanka wrote: > On 2015/03/02 21:54:24, shrike wrote: > > On 2015/03/02 17:40:51, shrike wrote: > > > Thank you - that worked. Remaining problem is I can't get the browser to > flag > > a > > > download as dangerous. My solution for that while putting the fix together > was > > > changing the code in download_target_determiner.cc to flag all files as > > > dangerous but obviously that's not an option in the build downloaded from > the > > > server. > > > > Despite this issue is it OK to push the change? > > If you have the build downloaded, it should be pretty easy to test. The file > types for which the dangerous prompt is triggered is in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow... > . The warning won't be triggered for explicit downloads ("Save link as.."). So > you need to construct something that'll act as an implicit download of a > dangerous file. For example: > > * Search Google for "filetype:swf". SWF is a dangerous file type. > * The results page will have lots of links to SWF files marked as [FLASH]. You > need to construct implicit download links from these. So pick one, right click > and choose inspect element. > * The anchor element will look like '<a href="[tracking redirect]" > onmousedown="[something]" data-href="[ACTUAL URL]">'. Remove the href and > onmousedown attributes, and rename "data-href" to "href". Then add another > attribute called "download". > * Your link will now look like '<a href="[ACTUAL URL]" download>'. > * Click this link. You should see a download warning. And feel free to land once you've verified that it's working.
On 2015/03/03 18:06:03, asanka wrote: > > * Search Google for "filetype:swf". SWF is a dangerous file type. > > * The results page will have lots of links to SWF files marked as [FLASH]. You > > need to construct implicit download links from these. So pick one, right click > > and choose inspect element. > > * The anchor element will look like '<a href="[tracking redirect]" > > onmousedown="[something]" data-href="[ACTUAL URL]">'. Remove the href and > > onmousedown attributes, and rename "data-href" to "href". Then add another > > attribute called "download". > > * Your link will now look like '<a href="[ACTUAL URL]" download>'. > > * Click this link. You should see a download warning. Thank you so much for the steps. With the filetype search I was able to find links that triggered the warning (was searching for swf files yesterday without that trick, with no luck). I also didn't realize it was possible to edit elements in the inspector - great to know! > And feel free to land once you've verified that it's working. I have verified the isolate build continues to work OK on OS X 10.9, and fixes the problem on OS X 10.10.
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/911303002/#ps80001 (title: "Resize download item textfield in xib rather than in code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/911303002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4ba4f927840199e4b84a530b43ad2de7402e157c Cr-Commit-Position: refs/heads/master@{#318927} |
