The intent in creating a door hanger in the Beta browser is • If a user opens the dev tools in Beta • A door hanger should immediately open for them • That let's them know there is a browser made specifically for developers • With a link to the dev browser product/download page. CTA: Learn more

cc'ing Joe - wanted to make sure you saw this issue. We need to trigger a doorhanger when the user opens the toolbox for the first time, in Beta. Worth considering timing, should we do this for Beta 34 or 35, as 34 would require a late uplift.

My opinion is that we shouldn't do this for the 9th, and we shouldn't crash-land beta. Sorry. Issues: l10n, nags can be annoying, sensitivity to late uplifts, doorhangers are not trivial.

Does this need to actually be a doorhanger, or could it be a notification bar in the devtools panel?

(In reply to Dave Camp (:dcamp) from comment #3) ... We ( Mozilla, the Desktop team, etc ) or consistently using the doorhanger for this sort of thing, so it feels very consistent to do so. Long term I would like a doorhanger to drop down on any Beta build the first time the user opens the toolbox. > Does this need to actually be a doorhanger, or could it be a notification > bar in the devtools panel? Is that easier for uplifts and l10n? Short term, it feels like the notification bar could work for Beta 34 - l10n doesn't seems to really care about devtools strings and the code we'd need to change is all in our codebase.

(In reply to Jeff Griffiths (:canuckistani) from comment #4) > We ( Mozilla, the Desktop team, etc ) or consistently using the doorhanger > for this sort of thing, so it feels very consistent to do so. Long term I > would like a doorhanger to drop down on any Beta build the first time the > user opens the toolbox. Actually, I think we've been rather inconsistent. As originally intended, doorhangers were meant to be applied to the urlbar only for site-specific messages and permission prompts, and we started moving old notification bars to this form. But we never really developed good UI for "messages from the browser". We use global notification bars in some places, and more recently doorhangers in others.

(In reply to Justin Dolske [:Dolske] from comment #5) > (In reply to Jeff Griffiths (:canuckistani) from comment #4) > > > We ( Mozilla, the Desktop team, etc ) or consistently using the doorhanger > > for this sort of thing, so it feels very consistent to do so. Long term I > > would like a doorhanger to drop down on any Beta build the first time the > > user opens the toolbox. > > Actually, I think we've been rather inconsistent. ... Sorry, the consistency I was referring to was that the first run experiences for Australis and now for Dev Edition use doorhangers, so for the user the most recent incidents where we've done something like this doorhangers are the 'norm'. I think you're right about the larger picture.

Any other information? Current requirements and questions, correct me if I'm wrong: * Show doorhanger ONLY once per profile when toolbox is opened, ONLY in beta -- is this release dependent (any version 34+ in beta), or version dependent (only Fx34/35)? should all beta releases have this notification? * Any design work done for this, or just waiting for the copy? Do we have a logo or any imagery for this? Any other doorhangers to be "inspired" by? * Do we have a URL to go to for downloads yet, or is that part of the copy?

Note from bug 1080015, use the "dark theme" to contrast with chrome dev tools -- is this the case? Should the style inherit from whatever the theme that's currently set, or should it always be "dark theme"?

rough draft

shorlander, is it possible to get some design work for this notification? jeff, is this for ANY version of beta?

Sure, any Beta. To be more detailed in how I think it should work: - users running beta should be prompted the first time they 'use a tool'. - users should be prompted the first time they use a tool after updating to a new version ( eg Beta 36, previous version was Beta 35 ) 'use a tool' means 1 of: - opening webide - opening the toolbox - triggering responsive design view - opening the Browser console In a month I'm going to log a bug about doing this same behaviour in release :)

first pass at styled door hanger

Hey Jordan! Looking good. As for the logo, we have a new one just for this browser! http://people.mozilla.org/~smartell/fx10/dev/fx-logo-developers.ai

Ah that is beautiful, will use that once we have a version in the codebase!

Animated gif of all scenarios -- WebIDE, opening the toolbox (for all 3 hosts), browser console and responsive design mode. Could probably use a UI/UX eye on it, as well as product

Here's the patch doing all of the magic. Tried to make it reusable, but like other doorhanger code, doesn't seem to get completely there. jryans, this touches mostly your code (webide, toolbox), so thinking you might be good to review this, or maybe you know someone who might be more appropriate? I'm not sure how the actual uplift and preference magic happens however.

To clarify in the animated gif, the line was commented out that sets the preference so that the doorhanger only appears once, for testing.

Comment on attachment 8512095 [details] [diff] [review] 1078539 Review of attachment 8512095 [details] [diff] [review]: ----------------------------------------------------------------- Can you rebase this? It doesn't apply with latest on fx-team.

all fixed up!

I noticed that the rounded corners have some "fuzzy" artifacts on OS X 10.9 with a non-Retina screen. It's less noticeable on a Retina screen, there is still what seems like a white border on the rounded part, which clashes a bit with the effect.

Yuck -- I don't have any 10.9 non-retina devices, but I'll check it out and see if I can fix it on a retina screen

The placement with Responsive Design mode is a bit odd since it covers the tab bar. May want to adjust this.

Comment on attachment 8512133 [details] [diff] [review] 1078539-dev-ed-promo-doorhanger.patch Review of attachment 8512133 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see the tweaked placements and learn about the logo decision first. ::: browser/app/profile/firefox.js @@ +1293,5 @@ > pref("browser.devedition.theme.enabled", false); > pref("browser.devedition.theme.showCustomizeButton", false); > > +// Developer edition promo preferences > +pref("devtools.devedition.promo.shown", false); I believe Joe is coordinating "turn on" patches in meta bug 1082509, so you may want to check with him about that step. But a bigger issue is where the change to enable the pref should be placed. There does not appear to be an obvious "Beta-only" prefs file, like there is for Aurora, so you may need more info from Firefox devs about where to make such a change. But again, that patch should be part of a separate bug that blocks the meta bug above. ::: browser/devtools/framework/dev-edition-promo/dev-edition-promo.css @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +.doorhanger-container { I believe (nearly?) all these selectors could be ID selectors instead of class, if you move the class to an ID. The classes are all unique, so why not? @@ +27,5 @@ > + margin: 5px 0px; > +} > + > +image.icon { > + background-image: url("chrome://branding/content/about-logo@2x.png"); Using this logo means you get logo of the currently running Firefox channel. Is there a specific "Dev Edition" Firefox logo that should be used? It seems like this should be fixed logo independent of what channel is currently running. @@ +59,5 @@ > + height: 30px; > + width: 450px; > +} > + > +.closebutton { Nit: Whether or not these become IDs, I would say drop the "button" from the class / ID. If you want "button" in the CSS for reading, you could do "button.close" for example. ::: browser/devtools/framework/toolbox.js @@ +16,5 @@ > let EventEmitter = require("devtools/toolkit/event-emitter"); > let Telemetry = require("devtools/shared/telemetry"); > let {getHighlighterUtils} = require("devtools/framework/toolbox-highlighter-utils"); > let HUDService = require("devtools/webconsole/hudservice"); > +let { showDoorhanger } = require("devtools/shared/doorhanger"); Nit: This file doesn't uses spaces when destructuring... (Someone make a common style choice!) @@ +25,5 @@ > Cu.import("resource:///modules/devtools/scratchpad-manager.jsm"); > Cu.import("resource:///modules/devtools/DOMHelpers.jsm"); > Cu.import("resource://gre/modules/Task.jsm"); > > + Nit: Extra line @@ +1635,5 @@ > _highlighterHidden: function() { > this.emit("highlighter-hide"); > + }, > + > + Nit: Extra line ::: browser/devtools/framework/toolbox.xul @@ +96,5 @@ > <splitter id="toolbox-console-splitter" class="devtools-horizontal-splitter" hidden="true" /> > <box minheight="75" flex="1" id="toolbox-panel-webconsole" collapsed="true" /> > </vbox> > </notificationbox> > + Nit: extra line ::: browser/devtools/responsivedesign/responsivedesign.jsm @@ +218,5 @@ > > // E10S: We should be using target here. See bug 1028234 > ResponsiveUIManager.emit("on", { tab: this.tab }); > + > + // Hook to dispaly promotional Developer Edition doorhanger. Only displayed once. Nit: dispaly -> display @@ +219,5 @@ > // E10S: We should be using target here. See bug 1028234 > ResponsiveUIManager.emit("on", { tab: this.tab }); > + > + // Hook to dispaly promotional Developer Edition doorhanger. Only displayed once. > + showDoorhanger({ window: this.mainWindow, type: "deveditionpromo" }); I think the placement is strange for this case, since it covers the tab bar. I've attached an example. ::: browser/devtools/webide/content/webide.js @@ +92,5 @@ > > this.setupDeck(); > + > + // Hook to display promotional Developer Edition doorhanger. Only displayed once. > + showDoorhanger({ window, type: "deveditionpromo" }); I think it would look better to place the promo below the main toolbar, but this one is less strange to me than the Responsive Design case. ::: browser/locales/en-US/chrome/browser/devtools/toolbox.dtd @@ +186,5 @@ > + > +<!-- LOCALIZATION NOTE (deveditionpromo.*) For the Developer Edition popup > + - in Beta to promote the Developer Edition. --> > + > +<!ENTITY deveditionpromo.title "Introducing Firefox Developer Edition"> What is the localization plan here? Is this landing straight to Beta timed with Dev Edition, or does it ride the trains to Beta?

Comment on attachment 8512133 [details] [diff] [review] 1078539-dev-ed-promo-doorhanger.patch Review of attachment 8512133 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +1293,5 @@ > pref("browser.devedition.theme.enabled", false); > pref("browser.devedition.theme.showCustomizeButton", false); > > +// Developer edition promo preferences > +pref("devtools.devedition.promo.shown", false); We would not want to land this to Nightly as-is, since I assume we don't want the promo to appear yet...

Comment on attachment 8512090 [details] devedpromo.gif LGTM, barring any pixel-level nitpicks other people have.

So we're going to have to crash land on beta for this, which is fairly scarey. I think we should do a couple of things: * Make it happen only on beta * Make it not happen if there is no l10n. Just don't bug the user. * Have at least 2 levels of review that carefully ask the question 'What if it goes wrong here?' and make sure it always fails-safe When you're ready for r? maybe I'll send a fx-devtools email saying - please everyone review this.

Needinfo: Lawrence Mandel and Lukas Blakk - Not expecting any info, but I think you should know about this request from marketing. CC: Axel and Flod so you're aware of the patch - It adds strings, clearly too late, particularly since is has to land on beta, so I'm proposing that we don't translate, fail-safe and don't show this nag for non-EN builds. For context, see comment 0, comment 7, comment 11, and attachment 8512090 [details].

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #28) > CC: Axel and Flod so you're aware of the patch - It adds strings, clearly > too late, particularly since is has to land on beta, so I'm proposing that > we don't translate, fail-safe and don't show this nag for non-EN builds. If you don't want this to be translated and show it only for en-US builds (not sure how, I can't spot anything in the current patch), I suggest to not add the strings to toolbox.dtd and hard-code them for Beta.

(In reply to Francesco Lodolo [:flod] from comment #29) > (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) > from comment #28) > > CC: Axel and Flod so you're aware of the patch - It adds strings, clearly > > too late, particularly since is has to land on beta, so I'm proposing that > > we don't translate, fail-safe and don't show this nag for non-EN builds. > > If you don't want this to be translated and show it only for en-US builds > (not sure how, I can't spot anything in the current patch), I suggest to not > add the strings to toolbox.dtd and hard-code them for Beta. We need to avoid showing EN strings in an otherwise non-EN build. I was thinking we could try/catch around the string lookup and do nothing on exception, but I'm not fixed on an implementation.

Marking this tracking for 34/35/36. Before this could be crash landed on Beta we need confirmation from QA that this doesn't show up for non EN builds, however I'd like to know if there's a plan to get l10n for this in the next beta (35) so that we're getting non-EN users onto this new channel/build as well. There's a lot of bugs that can arise from l10n users and we shouldn't be building up a channel of only EN.

There are so many things in the air for Nov 9, I'd rather not add this doorhanger on 35 to the list. Let's roll that into 36.

(In reply to Axel Hecht [:Pike] from comment #32) > There are so many things in the air for Nov 9, I'd rather not add this > doorhanger on 35 to the list. Let's roll that into 36. I agree. Both Joe and Justin have made arguments (comment 2, comment 5) that to me read as against landing this. We have had stability problems in both Firefox 32 and 33. I do not think that we should add more risk to Firefox 34 by taking on this new feature. I would prefer to see us make our announcement on Nov 9, see what the uptake is like, and then push the door hanger in a later release to prompt more people. In terms of when to show the door hanger, I'm not sure that we should nag a user every time they update (as per comment 11). We have made great efforts to make updates more silent. I would instead like to see a system that gives us the opportunity to prompt when we want rather than every time we release. ni on GMC as they should know about this proposed in product cross promotion for the developer browser.

Additional question: is the intent to show this in all beta locales, or only locales which are typically active on the beta channel?

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #33) > I agree. Both Joe and Justin have made arguments (comment 2, comment 5) that > to me read as against landing this. My comment was just about general UX and panels-vs-notification bars. I think what's happening here is mostly ok in that regard. Although it's a little weird as just a floating panel, not even an arrow panel (judging from attachment 8512090 [details]). If you haven't had UX input on that, it would be good to do so. (Also, does this really need to be mozilla-employee-confidential?)

Summary from daily standup. We're ok to continue with this. Clearing ni? GMC as Gavin commented in that meeting.

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #36) > Summary from daily standup. We're ok to continue with this. > Clearing ni? GMC as Gavin commented in that meeting. Correct. We're good to continue with getting this into 34 and 35. I have one outstanding question (asked above) about prompting every time the version bumps. I don't think this is a concern for the short term but would like us to question whether we should do this for more than a few releases.

Stephen - thoughts on the interaction or look of this notification?

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #37) > I have one outstanding question (asked above) about prompting every time the > version bumps. I don't think this is a concern for the short term but would > like us to question whether we should do this for more than a few releases. Is getting an updated beta is enough of an event for people to understand why they are getting nagged again at all?

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #39) > (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #37) > > I have one outstanding question (asked above) about prompting every time the > > version bumps. I don't think this is a concern for the short term but would > > like us to question whether we should do this for more than a few releases. > > Is getting an updated beta is enough of an event for people to understand > why they are getting nagged again at all? Just to be clear: this would be ~ every 6 weeks, right? I don't want to nag more than once inside the same train cycle. Aside: success to me for this looks at a couple of things: * beta vs dev edition population size that have used the tools at least once * dev edition population, and the sub-set of them with dev edition as their default browser I think the future of this promotion depends a lot on those numbers, hard to predict at this point. Ask me in February.

(In reply to Jeff Griffiths (:canuckistani) from comment #40) > (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) > from comment #39) > > (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #37) > > > I have one outstanding question (asked above) about prompting every time the > > > version bumps. I don't think this is a concern for the short term but would > > > like us to question whether we should do this for more than a few releases. > > > > Is getting an updated beta is enough of an event for people to understand > > why they are getting nagged again at all? > > Just to be clear: this would be ~ every 6 weeks, right? I don't want to nag > more than once inside the same train cycle. Seems like getting prompted just once per profile should be enough. I don't see the reason why we would want to re-prompt every 6 weeks.

Agreed on the once-per-profile ping. Maybe in a few cycles we can reping with the notification for new beta users, or reminders for the long-time beta users. Outstanding questions/confirmations: * So no localized strings, just hardcoded english -- how do we get this only in en-US builds? * Best way to turn this on in v34/35? An option at the build level to set a preference if the build is a candidate to be potentially shown the notification? * Is the dev edition logo in m-c yet anywhere? And if it is, is it built on non-DevEdition, unlike our other brand logos? Still trying to figure out the border issue that jryans pointed out.

Also, this should really get a designer's opinion of this, I am not even remotely a designer.

Note, if an arrow panel is wanted, we need something for it to be pointing/anchored to in all 4 scenarios, and the arrows didn't really make sense in those contexts

Note that there was a suggestion of different copy in bug 1080015.

Different copy to be added from bug 1080015: Using dev tools in your browser? Download Firefox Developer Edition, our first browser made just for you. Learn more »

Updated in patch: * Fixed nits, element id/class names as per jryans comments from comment #24; outstanding questions regarding deployment/image still in comment * Hardcode strings (comment #29) * Updated copy from bug 1080015 * Only display if `general.useragent.locale` set to "en-US" -- possibly a better way of detecting locale? comment #30 TODO: * Fix white artifacts in corners (comment #21) * Change placement in some scenarios -- this is a bit more fine grained, and would just be margin adjustments, which could change based on themes/dimensions, unless it's anchored, which is still trusting certain items are there. Basically, I'm hesitant to change that part, but will do if its really needed, especially since I think most users will access the toolbox before diving straight into responsive design/webide/browser console, but could be wrong. NI?: * How can we configure which release channels to display this? (comment #27) * The Dev Edition brand logo needs to be in m-c, and available to beta release channel (comment #13) * UX/Design review * What URL should open if clicking "Learn more"? * Do we need a better method of testing locale than `general.useragent.locale`?

chrome registry getSelectedLocale("global") is the best way to get a sanitized value for the current locale on desktop.

Thanks, Axel -- updated patch to use that method instead.

Comment on attachment 8513814 [details] [diff] [review] 1078539-dev-ed-promo-doorhanger.patch Review of attachment 8513814 [details] [diff] [review]: ----------------------------------------------------------------- The things you have updated look good to me. I want to see another version once your remaining questions are answered. Instead of trying to juggle a bunch random margins for each situation, could the doorhanger take an element inside the window that it should be positioned on top of (probably still with the 20px it has right now)? That way we could target (for example) WebIDE's main content area or the browser tab content area for Responsive Design for better positioning. ::: browser/devtools/framework/dev-edition-promo/dev-edition-promo.css @@ +25,5 @@ > + font-family: Open Sans, sans-serif; > + font-size: 0.9em; > + width: 300px; > + display: block; > + margin: 5px 0px 0px 0px; Nit: Drop px for 0 ::: browser/devtools/framework/dev-edition-promo/dev-edition-promo.xul @@ +13,5 @@ > + <vbox id="doorhanger-container"> > + <hbox flex="1" id="top-panel"> > + <image id="icon" /> > + <vbox id="info"> > + <h1>Using dev tools in your browser?</h1> I would suggest changing: "dev tools" -> DevTools to be consistent with the way we use the term in other places.

Comment on attachment 8513823 [details] [diff] [review] 1078539-dev-ed-promo-doorhanger.patch Review of attachment 8513823 [details] [diff] [review]: ----------------------------------------------------------------- Same comments for this version.

Thanks jryans, pinging alainez from bug 1080015 for the copy change "dev tools" -> "DevTools". Regarding the positioning on another element, that sounds good -- should be able to take a selector (instead of `window` on these XUL docs) and attach on that, which should be good.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #53) > Thanks jryans, pinging alainez from bug 1080015 for the copy change "dev > tools" -> "DevTools". If you can fit in 'Developer Tools' that would be better, otherwise 'DevTools'

+1 to Developer Tools if it fits. DevTools if it doesn't

* Changed title to use "Developer Tools" * Added new logo * Fixed jryans nits (comment #51) * repositioned the webide/responsive popups to be anchored inside the main window

What it looks like now in all four contexts.

Outstanding NI?: * How can we configure which release channels to display this? (comment #27) * UX/Design review * What URL should open if clicking "Learn more"?

Comment on attachment 8514599 [details] [diff] [review] 1078539-dev-ed-promo-doorhanger.patch Review of attachment 8514599 [details] [diff] [review]: ----------------------------------------------------------------- Cool, the code here looks good to me. r+ assuming all the outstanding questions are resolved.

Possible way to enable this on beta -- set a preference if the update channel is in beta (confirmation on better ways to do this?) This also allows us to test it by just setting this preference on. This preference must be true, and `devtools.devedition.promo.shown` must be false to display the promo. Need some feedback if this is an appropriate way of landing this to be displayed on beta only. Tested the preference works, but have not yet built with the update channel set. #ifdef MOZ_UPDATE_CHANNEL == beta pref("devtools.devedition.promo.enabled", true); #else pref("devtools.devedition.promo.enabled", false); #endif

Pinging Jeff w/r/t what should the URL be for the page opened by clicking on "Learn more"

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #58) > Outstanding NI?: > * UX/Design review Have some thoughts, will post tomorrow.

This looks good, I do think it's odd that we are using a doorhanger but not attaching it to anything. We usually use the arrow to point to the relevant item. In this case that would be the DevTools. I would suggest, where possible, positioning the doorhanger outside of the DevTools and pointing to them directly.

Gijs, can you help with comment 60?

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #64) > Gijs, can you help with comment 60? I expect going off the update channel is fair; less sure about using an ifdef for that; there's an update-channel pref, although I'm not sure we care about users that started running nightly and then switched that pref (besides, they'd be running beta within... a few months... or so...). Jared, do you know for sure?

I don't know "for sure", but this seems like a fine route to take. I prefer the patch's approach of placing the ifdefs in the prefs file as opposed to keying off of the update channel pref, as we may want to tweak the conditions that we expose this doorhanger.

@gijs/@jaws: Ah yes, `app.update.channel` gives the same information; that should work. The build option in the ifdef would prevent the scenario of a user changing their release channel in about:config, but that seems like a pretty uncommon thing to do? But a good point, we may want to change the scenarios other than update channel in which we display; I'll leave this as is for now. @shorlander: anchoring with an arrow pointing is possible in some contexts, but not in others (browser console, webIDE, have their own windows) -- I'll see if I can get this working in the contexts it makes sense (dev tools for bottom/side/window host and browser console)

The url should be the Product landing page, I can't for the life of me remember what that is. needinfo'ing Arcadio.(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #61) > Pinging Jeff w/r/t what should the URL be for the page opened by clicking on > "Learn more" https://mozila.org/firefox/developer

Updated URL to https://mozila.org/firefox/developer

Rebased against fx-team

Fixed typo in URL, made it a pref, retested it all, sending it to jryans for one last review

Patch rebased for landing on mozilla-aurora. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4123c9ddf29a Need to manually test this locally as well.

New try for aurora push, testing things other than devtools: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0c2d1d66491a

Patch for mozilla-beta

aurora try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0c2d1d66491a aurora patch: attachment 8517010 [details] [diff] [review] beta try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=18d5c22ab44a beta patch: attachment 8517118 [details] [diff] [review] Manually tested in aurora and beta builds as well. The aurora and beta patches need to be landed once dev edition is live -- pinging jwalker for that. The current patch[0] can be landed in m-c. https://bugzilla.mozilla.org/attachment.cgi?id=8516970

I did some exploratory testing on this notification panel using the try-builds from comment 75 on all platforms (Windows 7 64bit, Mac OS X 10.9.5 and Ubuntu 14.04 32bit). During testing I encountered a few UI issues. Will this be the final design for the notification doorhanger? More details about the testing and issues found can be found in this etherpad: https://mozqa.etherpad.mozilla.org/NotificationDevTools. Please leave comments inline for all the issues or potential issues.

Waiting for account access on the mozqa etherpad to check it out

I think the right timing is to take this change in 34 beta8, which is scheduled to release on Nov 11. Can you confirm?

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #80) > I think the right timing is to take this change in 34 beta8, which is > scheduled to release on Nov 11. Can you confirm? Confirmed.

Making changes based on the feedback from QA, so will add new patches for aurora/beta/fx-team.

Just an update from today from QA notes[0]: * Remove embossed borders on linux/window buttons * Change font size on linux only to not overflow * Fix white corners on the rounded edges in all browsers Remaining: * Flicker on Windows 7 * On all builds 34-36 on linux, browser console/webide does not open, but triggers the switch. Also happens on OSX 10.9 (not on 10.8, which is unfortunately what I have) Pulling down and building on linux to deal with changes as the builds aren't cutting it. Not making patches until this is fixed, as will require rebasing and rebuilding from beta/aurora [0]https://mozqa.etherpad.mozilla.org/NotificationDevTools

After all day on this, creating a linux development environment to actually debug these issues, and digging into webide and browser console, adding a 500ms delay seemed to just work -- not an elegant fix by any means, but for a temporary promotional popup, this seems to be a fine hacked solution just to get the necessary patches tested and landed in time. If this sounds good, I'll build out patches for each of the releases, which takes about 30m a piece. Any objections to the hacked approach, let me know, otherwise I'll start building in the (EST) morning.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #84) > After all day on this, creating a linux development environment to actually > debug these issues, and digging into webide and browser console, adding a > 500ms delay seemed to just work -- not an elegant fix by any means, but for > a temporary promotional popup, this seems to be a fine hacked solution just > to get the necessary patches tested and landed in time. Does a 0ms delay work? If it's actually time-dependent (rather than just requiring unwinding the stack via a spin of the event loop), then that's a much more fragile solution.

A 0ms delay does not work unfortunately

Using onfocus events for browser console/webide, since there were a lot of timing issues with that. this seems to work all the time now! Also the style fixes from the previous comment from me is in this as well. Building out aurora and beta versions now.

Waiting for try to open to get builds.

Waiting for try to open to get builds.

Can we back out the original patch[0] on fx-team and land attachment 8518246 [details] [diff] [review] in its place? After landing for testing, QA found some issues and those should now be patched, but as aurora/beta are still unlanded, and full patches, so is the fx-team patch. For Aurora/Beta patches, awaiting try to open up to push those so we can do one more round (hopefully) of testing those builds before dropping those patches. [0]https://hg.mozilla.org/mozilla-central/rev/224f20dce9d0

https://hg.mozilla.org/integration/fx-team/rev/c2ced33d9744 https://hg.mozilla.org/integration/fx-team/rev/3e45eeacb231

Thanks Ryan! Aurora attachment 8518341 [details] [diff] [review] Try Build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5d6960a5710 Beta attachment 8518340 [details] [diff] [review] Try Build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f748c79c6e76 Pinging you again, Bogdan -- all those issues should be fixed on all platforms.

A quick look, the beta build on Linux has all the issues fixed, so this is promising.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #92) > Thanks Ryan! > > Aurora > attachment 8518341 [details] [diff] [review] > Try Build: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5d6960a5710 > > Beta > attachment 8518340 [details] [diff] [review] > Try Build: > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f748c79c6e76 > > Pinging you again, Bogdan -- all those issues should be fixed on all > platforms. The issue with the notification that flickers is still not fixed, the rest are fixed. Though I found new issues that were described in the same etherpad as above [0]. Did not find any new issues on Mac OS X (only on Windows and Ubuntu) so that build looks nice from QA point of view. [0] https://mozqa.etherpad.mozilla.org/NotificationDevTools?

* Found a fix for the popup requiring double clicks to close on Windows/Linux * Bogdan, can you clarify the issue with the separate windows and the popup appearing again? Due to triggering on window focus, if setting the preference for it to show again, it makes sense for it to show again, but don't think this is an issue (as someone playing with the config, that could happen). * Looking into the Windows flickering issue If the separate windows thing is not an issue, I'll render out more builds once I find a fix for the Windows flickering, as I have a fix for the double clicks on Windows/Linux one. https://mozqa.etherpad.mozilla.org/NotificationDevTools

Beta builds: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=90dffa81e6e2 patch: attachment 8519122 [details] [diff] [review] Aurora builds: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=33db88748b32 patch: attachment 8519121 [details] [diff] [review] fx-team With the main patch already landed on fx-team, there is a small fix ontop of that for a second patch ( attachment 8519123 [details] [diff] [review] ). The aurora and beta patches, since not yet landed, these two patches are merged together. Can we get a checkin on attachment 8519123 [details] [diff] [review] to fx-team? Pinging Bogdan to be aware of the builds starting for hopefully one last review. The double-click popup on windows/linux is now fixed, and a delay has been added to displaying the popup to hopefully combat the Windows 7 64bit flickering (I cannot recreate on my VM).

I tested using try builds from above on Windows 7 64bit, Windows 8.1 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.5 all the issues are fixed and no new issues were found.

Great. So looks like we just need to land the aurora and beta patches accordingly, and then the latest patch for fx-team (attachment 8519123 [details] [diff] [review]).

If this is going to make beta8, it needs to land today. If this is ready for Beta, please submit an approval requests ASAP.

Comment on attachment 8519121 [details] [diff] [review] 1078539-aurora.patch Approval Request Comment [Feature/regressing bug #]: This feature [User impact if declined]: No doorhanger promoting dev edition when aurora moves up to beta on the next release cycle [Describe test coverage new/current, TBPL]: try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=90dffa81e6e2, tested by :bogdan_maris in comment #101 [Risks and why]: None [String/UUID change made/needed]: None

Comment on attachment 8519122 [details] [diff] [review] 1078539-beta.patch Approval Request Comment [Feature/regressing bug #]: This feature [User impact if declined]: No doorhanger promoting dev edition in beta [Describe test coverage new/current, TBPL]: try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=90dffa81e6e2, tested by :bogdan_maris in comment #101 [Risks and why]: None [String/UUID change made/needed]: None

Comment on attachment 8519121 [details] [diff] [review] 1078539-aurora.patch Previous try build link for aurora was incorrect, updated try path: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=33db88748b32

Patch attachment 8519123 [details] [diff] [review] still needs to land on fx-team as well.

Comment on attachment 8519122 [details] [diff] [review] 1078539-beta.patch Somewhat big for this stage in beta but time dependent and relatively self contained. Let's get this into beta8 as planned. Beta+

This missed the boat for beta8, unfortunately. https://hg.mozilla.org/releases/mozilla-beta/rev/e7f8aa528841

Marking for final verification in 34 Beta 9.

Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: The dev edition promo will be shown when beta9 lands, which some work is still needed in dev edition, and this promo is premature at this point [Describe test coverage new/current, TBPL]: none [Risks and why]: Premature promotion for the dev edition browser coming in beta9 otherwise [String/UUID change made/needed]: none

Quick smoke test: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=44f1403dc8ab

Comment on attachment 8522427 [details] [diff] [review] 1078539-beta-disable-dev-edition-promo.patch Disappointing that we took this very late and then had to turn if off. Still, better that we turn if off than promote a browser that we're not ready to promote. Beta+

Looks like this missed beta9 due to missing status firefox34 flag -- anyway this can get landed super last minute in beta9?

This is going into beta 9 build 2 later tonight. So, as I understand this, I'm going to be making sure that the doorhanger isn't there.

Previously pushed to beta9 build 2: http://hg.mozilla.org/releases/mozilla-beta/rev/1242fc159d04 Correct -- by default, the `devtools.devedition.promo.enabled` should be false, and when opening the dev tools, responsive design mode, webide or browser console, there should be -no- notification.

Jordan, the weird thing is I tried Beta 9 build 1 just now, the en-US Mac build, and did not see the doorhanger come up when I opened the console, WebIDE, or toolbox -- in theory, it should be in build 1.

I suspect build1 worked fine because the code is #ifdef MOZ_UPDATE_CHANNEL == beta instead of #if MOZ_UPDATE_CHANNEL == beta cf http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/searchplugins/google.xml#19

Nick, can confirm that is an issue. Rebuilding on the aurora branch with `--enable-update-channel=beta`, does indeed not display the notification. Made a patch fixing this for the aurora branch, for uplift. I don't think this the end of the world if it doesn't make aurora before uplift, but will have to make sure this lands on beta Fx35 at some point. Approval Request Comment [Feature/regressing bug #]: this [User impact if declined]: Users will not be alerted about the dev edition when they upgrade their beta release to Fx35 [Describe test coverage new/current, TBPL]: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7fec50455160 [Risks and why]: no risks, just no notification come betatime [String/UUID change made/needed]: none

[Tracking Requested - why for this release]:

Fix for fx-team/Fx36 before uplift

Is this still targeted for Beta 34? status-firefox34 still shows as fixed, but as far as I can tell we're only targeting this for Beta 35. Can anyone confirm this?

It's been disabled in the current Firefox 34, and yes this is patched in Fx36, and awaiting checkin of aurora fx35 where it'll actually be used. The Fx36 patch isn't triggered until it's pushed to beta channel.

Backed out for OSX 10.8 mochitest-dt orange. https://treeherder.mozilla.org/ui/logviewer.html#?job_id=416767&repo=mozilla-aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/14db369bb36e

Passing locally on my OSX 10.8, repushing to try https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=daaab84865d3

OSX 10.8 tests don't run by default on Try. That said, it looks like mochitest-dt went green on its own after the 2 straight occurrences this got backed out for, so I'll just re-land. *sigh*

Second time's the charm. https://hg.mozilla.org/releases/mozilla-aurora/rev/62cd0230068f

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 Mozilla/5.0 (Windows NT 6.1; rv:35.0) Gecko/20100101 Firefox/35.0 Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0 The doorhanger notification is correctly implemented and displayed for the devtools on Firefox 35 beta 1, build ID: 20141201162954. Verified on Windows 7 64-bit, Windows 8.1 64-bit (MS Pro 2 device), Ubuntu 14.04 64-bit and Mac OS X 10.9.5.

Tested on Windows 8.1 64bit, Ubuntu 14.04 32bit and Mac OS X 10.9.5 using Firefox 36 beta 2 build 1 (buildID : 20150120155007), the doorhanger notification is displayed correctly and no new issues were found.