From 7aa2c6c30fa407dc2bad994c8ba8143adcd64c3b Mon Sep 17 00:00:00 2001 From: Lorne Mitchell Date: Wed, 3 Apr 2019 03:50:10 +0000 Subject: [PATCH] DevTools: Enabled check_localization presubmit script * Enabled the check_localization presubmit script. * Fixed localization issues caught by the check_localization presubmit script. * Updated the check_localization script to allow contatenation of non-alphabetic strings with localized strings. * For example, ls`Status Code` + ": " is a valid concatenation. This allows for decorations to be concatenated with localized strings. Change-Id: I741940c9ebdac363ac0ccad3f7de20d508204e2b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1544878 Auto-Submit: Lorne Mitchell Reviewed-by: Joel Einbinder Reviewed-by: Pavel Feldman Commit-Queue: Pavel Feldman Cr-Original-Commit-Position: refs/heads/master@{#647134} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 7a329cb73b0b5d5cddb89a34fd7da6cb1a3c87dd --- PRESUBMIT.py | 2 +- front_end/audits2/Audits2Controller.js | 6 +- front_end/audits2/Audits2StatusView.js | 3 +- front_end/coverage/CoverageView.js | 3 +- .../network/ResourceWebSocketFrameView.js | 21 ++++--- front_end/sdk/NetworkManager.js | 19 ++++-- front_end/sources/GoToLineQuickOpen.js | 6 +- front_end/sources/SourcesPanel.js | 3 +- front_end/sources/ThreadsSidebarPane.js | 2 +- front_end/timeline/TimelinePanel.js | 6 +- front_end/timeline/TimelineUIUtils.js | 7 ++- scripts/check_localizability.js | 63 ++++++++++++++----- 12 files changed, 88 insertions(+), 53 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 5fadb7aa57..68de82a544 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -202,7 +202,7 @@ def CheckChangeOnUpload(input_api, output_api): results = [] results.extend(_CheckBuildGN(input_api, output_api)) results.extend(_CheckFormat(input_api, output_api)) - # results.extend(_CheckDevtoolsLocalization(input_api, output_api)) + results.extend(_CheckDevtoolsLocalization(input_api, output_api)) results.extend(_CheckDevtoolsStyle(input_api, output_api)) results.extend(_CompileDevtoolsFrontend(input_api, output_api)) results.extend(_CheckConvertSVGToPNGHashes(input_api, output_api)) diff --git a/front_end/audits2/Audits2Controller.js b/front_end/audits2/Audits2Controller.js index 6ef05d79fa..6869d7c22f 100644 --- a/front_end/audits2/Audits2Controller.js +++ b/front_end/audits2/Audits2Controller.js @@ -97,8 +97,7 @@ Audits2.AuditController = class extends Common.Object { const inspectedURL = mainTarget && mainTarget.inspectedURL(); if (inspectedURL && !/^(http|chrome-extension)/.test(inspectedURL)) { return Common.UIString( - 'Can only audit HTTP/HTTPS pages and Chrome extensions. ' + - 'Navigate to a different page to start an audit.'); + 'Can only audit HTTP/HTTPS pages and Chrome extensions. Navigate to a different page to start an audit.'); } return null; @@ -179,8 +178,7 @@ Audits2.AuditController = class extends Common.Object { let helpText = ''; if (hasActiveServiceWorker) { helpText = Common.UIString( - 'Multiple tabs are being controlled by the same service worker. ' + - 'Close your other tabs on the same origin to audit this page.'); + 'Multiple tabs are being controlled by the same service worker. Close your other tabs on the same origin to audit this page.'); } else if (!hasAtLeastOneCategory) { helpText = Common.UIString('At least one category must be selected.'); } else if (unauditablePageMessage) { diff --git a/front_end/audits2/Audits2StatusView.js b/front_end/audits2/Audits2StatusView.js index af4105fb5e..149eb45e8a 100644 --- a/front_end/audits2/Audits2StatusView.js +++ b/front_end/audits2/Audits2StatusView.js @@ -223,8 +223,7 @@ Audits2.StatusView = class { this._statusText.createChild('p').createTextChild(Common.UIString('Ah, sorry! We ran into an error.')); if (Audits2.StatusView.KnownBugPatterns.some(pattern => pattern.test(err.message))) { const message = Common.UIString( - 'Try to navigate to the URL in a fresh Chrome profile without any other tabs or ' + - 'extensions open and try again.'); + 'Try to navigate to the URL in a fresh Chrome profile without any other tabs or extensions open and try again.'); this._statusText.createChild('p').createTextChild(message); } else { this._renderBugReportBody(err, this._inspectedURL); diff --git a/front_end/coverage/CoverageView.js b/front_end/coverage/CoverageView.js index c4816ee2b5..f8d943422d 100644 --- a/front_end/coverage/CoverageView.js +++ b/front_end/coverage/CoverageView.js @@ -76,8 +76,7 @@ Coverage.CoverageView = class extends UI.VBox { if (this._startWithReloadButton) { const reloadButton = UI.createInlineButton(UI.Toolbar.createActionButtonForId('coverage.start-with-reload')); message = UI.formatLocalized( - 'Click the record button %s to start capturing coverage.\n' + - 'Click the reload button %s to reload and start capturing coverage.', + 'Click the record button %s to start capturing coverage.\nClick the reload button %s to reload and start capturing coverage.', [recordButton, reloadButton]); } else { message = UI.formatLocalized('Click the record button %s to start capturing coverage.', [recordButton]); diff --git a/front_end/network/ResourceWebSocketFrameView.js b/front_end/network/ResourceWebSocketFrameView.js index 4b42276725..89ba17c443 100644 --- a/front_end/network/ResourceWebSocketFrameView.js +++ b/front_end/network/ResourceWebSocketFrameView.js @@ -115,9 +115,10 @@ Network.ResourceWebSocketFrameView = class extends UI.VBox { * @return {string} */ static opCodeDescription(opCode, mask) { - const rawDescription = Network.ResourceWebSocketFrameView.opCodeDescriptions[opCode] || ''; - const localizedDescription = Common.UIString(rawDescription); - return Common.UIString('%s (Opcode %d%s)', localizedDescription, opCode, (mask ? ', mask' : '')); + const localizedDescription = Network.ResourceWebSocketFrameView.opCodeDescriptions[opCode] || ''; + if (mask) + return ls`${localizedDescription} (Opcode ${opCode}, mask)`; + return ls`${localizedDescription} (Opcode ${opCode})`; } /** @@ -230,12 +231,12 @@ Network.ResourceWebSocketFrameView.OpCodes = { Network.ResourceWebSocketFrameView.opCodeDescriptions = (function() { const opCodes = Network.ResourceWebSocketFrameView.OpCodes; const map = []; - map[opCodes.ContinuationFrame] = 'Continuation Frame'; - map[opCodes.TextFrame] = 'Text Message'; - map[opCodes.BinaryFrame] = 'Binary Message'; - map[opCodes.ContinuationFrame] = 'Connection Close Message'; - map[opCodes.PingFrame] = 'Ping Message'; - map[opCodes.PongFrame] = 'Pong Message'; + map[opCodes.ContinuationFrame] = ls`Continuation Frame`; + map[opCodes.TextFrame] = ls`Text Message`; + map[opCodes.BinaryFrame] = ls`Binary Message`; + map[opCodes.ContinuationFrame] = ls`Connection Close Message`; + map[opCodes.PingFrame] = ls`Ping Message`; + map[opCodes.PongFrame] = ls`Pong Message`; return map; })(); @@ -272,7 +273,7 @@ Network.ResourceWebSocketFrameNode = class extends DataGrid.SortableDataGridNode } else if (frame.opCode === Network.ResourceWebSocketFrameView.OpCodes.BinaryFrame) { length = Number.bytesToString(base64ToSize(frame.text)); - description = 'Binary Message'; + description = Network.ResourceWebSocketFrameView.opCodeDescriptions[frame.opCode]; } else { dataText = description; diff --git a/front_end/sdk/NetworkManager.js b/front_end/sdk/NetworkManager.js index 1e598a3542..312b2cdedd 100644 --- a/front_end/sdk/NetworkManager.js +++ b/front_end/sdk/NetworkManager.js @@ -858,8 +858,7 @@ SDK.NetworkDispatcher = class { if (shouldReportCorbBlocking) { const message = Common.UIString( - `Cross-Origin Read Blocking (CORB) blocked cross-origin response %s with MIME type %s. ` + - `See https://www.chromestatus.com/feature/5629709824032768 for more details.`, + `Cross-Origin Read Blocking (CORB) blocked cross-origin response %s with MIME type %s. See https://www.chromestatus.com/feature/5629709824032768 for more details.`, networkRequest.url(), networkRequest.mimeType); this._manager.dispatchEventToListeners( SDK.NetworkManager.Events.MessageGenerated, @@ -868,10 +867,18 @@ SDK.NetworkDispatcher = class { if (Common.moduleSetting('monitoringXHREnabled').get() && networkRequest.resourceType().category() === Common.resourceCategories.XHR) { - const message = Common.UIString( - (networkRequest.failed || networkRequest.hasErrorStatusCode()) ? '%s failed loading: %s "%s".' : - '%s finished loading: %s "%s".', - networkRequest.resourceType().title(), networkRequest.requestMethod, networkRequest.url()); + let message; + const failedToLoad = networkRequest.failed || networkRequest.hasErrorStatusCode(); + if (failedToLoad) { + message = Common.UIString( + '%s failed loading: %s "%s".', networkRequest.resourceType().title(), networkRequest.requestMethod, + networkRequest.url()); + } else { + message = Common.UIString( + '%s finished loading: %s "%s".', networkRequest.resourceType().title(), networkRequest.requestMethod, + networkRequest.url()); + } + this._manager.dispatchEventToListeners( SDK.NetworkManager.Events.MessageGenerated, {message: message, requestId: networkRequest.requestId(), warning: false}); diff --git a/front_end/sources/GoToLineQuickOpen.js b/front_end/sources/GoToLineQuickOpen.js index d5a6986d11..e4efc99e58 100644 --- a/front_end/sources/GoToLineQuickOpen.js +++ b/front_end/sources/GoToLineQuickOpen.js @@ -29,11 +29,9 @@ Sources.GoToLineQuickOpen = class extends QuickOpen.FilteredListWidget.Provider const position = this._parsePosition(query); if (!position) return Common.UIString('Type a number to go to that line.'); - let text = Common.UIString('Go to line ') + position.line; if (position.column && position.column > 1) - text += Common.UIString(' and column ') + position.column; - text += '.'; - return text; + return ls`Go to line ${position.line} and column ${position.column}.`; + return ls`Go to line ${position.line}.`; } /** diff --git a/front_end/sources/SourcesPanel.js b/front_end/sources/SourcesPanel.js index bc6cfb60de..718570888b 100644 --- a/front_end/sources/SourcesPanel.js +++ b/front_end/sources/SourcesPanel.js @@ -460,8 +460,7 @@ Sources.SourcesPanel = class extends UI.Panel { _pauseOnExceptionEnabledChanged() { const enabled = Common.moduleSetting('pauseOnExceptionEnabled').get(); this._pauseOnExceptionButton.setToggled(enabled); - this._pauseOnExceptionButton.setTitle( - Common.UIString(enabled ? 'Don\'t pause on exceptions' : 'Pause on exceptions')); + this._pauseOnExceptionButton.setTitle(enabled ? ls`Don't pause on exceptions` : ls`Pause on exceptions`); this._debugToolbarDrawer.classList.toggle('expanded', enabled); } diff --git a/front_end/sources/ThreadsSidebarPane.js b/front_end/sources/ThreadsSidebarPane.js index e9c36de70b..fdeef9ac40 100644 --- a/front_end/sources/ThreadsSidebarPane.js +++ b/front_end/sources/ThreadsSidebarPane.js @@ -45,7 +45,7 @@ Sources.ThreadsSidebarPane = class extends UI.VBox { } function updatePausedState() { - pausedState.textContent = Common.UIString(debuggerModel.isPaused() ? 'paused' : ''); + pausedState.textContent = debuggerModel.isPaused() ? ls`paused` : ''; } /** diff --git a/front_end/timeline/TimelinePanel.js b/front_end/timeline/TimelinePanel.js index f086c62668..46e4019ff8 100644 --- a/front_end/timeline/TimelinePanel.js +++ b/front_end/timeline/TimelinePanel.js @@ -655,13 +655,11 @@ Timeline.TimelinePanel = class extends UI.Panel { const reloadButton = UI.createInlineButton(UI.Toolbar.createActionButtonForId('timeline.record-reload')); centered.createChild('p').appendChild(UI.formatLocalized( - 'Click the record button %s or hit %s to start a new recording.\n' + - 'Click the reload button %s or hit %s to record the page load.', + 'Click the record button %s or hit %s to start a new recording.\nClick the reload button %s or hit %s to record the page load.', [recordButton, recordKey, reloadButton, reloadKey])); centered.createChild('p').appendChild(UI.formatLocalized( - 'After recording, select an area of interest in the overview by dragging.\n' + - 'Then, zoom and pan the timeline with the mousewheel or %s keys.\n%s', + 'After recording, select an area of interest in the overview by dragging.\nThen, zoom and pan the timeline with the mousewheel or %s keys.\n%s', [navigateNode, learnMoreNode])); this._landingPage.show(this._statusPaneContainer); diff --git a/front_end/timeline/TimelineUIUtils.js b/front_end/timeline/TimelineUIUtils.js index eac2ac7b74..1426faddec 100644 --- a/front_end/timeline/TimelineUIUtils.js +++ b/front_end/timeline/TimelineUIUtils.js @@ -556,10 +556,13 @@ Timeline.TimelineUIUtils = class { break; } case recordType.ParseHTML: { + const startLine = event.args['beginData']['startLine']; const endLine = event.args['endData'] && event.args['endData']['endLine']; const url = Bindings.displayNameForURL(event.args['beginData']['url']); - detailsText = Common.UIString( - '%s [%s\u2026%s]', url, event.args['beginData']['startLine'] + 1, endLine >= 0 ? endLine + 1 : ''); + if (endLine >= 0) + detailsText = Common.UIString('%s [%s\u2026%s]', url, startLine + 1, endLine + 1); + else + detailsText = Common.UIString('%s [%s\u2026]', url, startLine + 1); break; } case recordType.CompileModule: diff --git a/scripts/check_localizability.js b/scripts/check_localizability.js index 3cbe973bfd..be7fe4c171 100644 --- a/scripts/check_localizability.js +++ b/scripts/check_localizability.js @@ -153,19 +153,48 @@ function getLocation(node) { return ''; } +function buildConcatenatedNodesList(node, nodes) { + if (!node) + return; + if (node.left === undefined && node.right === undefined) { + nodes.push(node); + return; + } + buildConcatenatedNodesList(node.left, nodes); + buildConcatenatedNodesList(node.right, nodes); +} + /** * Recursively check if there is concatenation to localization call. + * Concatenation is allowed between localized strings and non-alphabetic strings. + * It is not allowed between a localized string and a word. + * Example (allowed): ls`Status Code` + ": " + * Example (disallowed): ls`Status` + " Code" + ": " */ -function checkConcatenation(node, filePath, errors) { - if (node !== undefined && node.type === esprimaTypes.BI_EXPR && node.operator === '+') { - const code = escodegen.generate(node); - if (isLocalizationCall(node.left) || isLocalizationCall(node.right)) { - addError( - `${filePath}${getLocation(node)}: string concatenation should be changed to variable substitution with ls: ${ - code}`, - errors); - } else { - [node.left, node.right].forEach(node => checkConcatenation(node, filePath, errors)); +function checkConcatenation(parentNode, node, filePath, errors) { + function isWord(node) { + return (node.type === 'Literal' && !!node.value.match(/[a-z]/i)); + } + function isConcatenation(node) { + return (node !== undefined && node.type === esprimaTypes.BI_EXPR && node.operator === '+'); + } + + if (isConcatenation(parentNode)) + return; + + if (isConcatenation(node)) { + let concatenatedNodes = []; + buildConcatenatedNodesList(node, concatenatedNodes); + const hasLocalizationCall = !!concatenatedNodes.find(currentNode => isLocalizationCall(currentNode)); + if (hasLocalizationCall) { + const hasAlphabeticLiteral = !!concatenatedNodes.find(currentNode => isWord(currentNode)); + if (hasAlphabeticLiteral) { + const code = escodegen.generate(node); + addError( + `${filePath}${ + getLocation(node)}: string concatenation should be changed to variable substitution with ls: ${code}`, + errors); + } } } } @@ -186,6 +215,10 @@ function checkFunctionArgument(functionName, argumentIndex, node, filePath, erro if (node !== undefined && node.type === esprimaTypes.CALL_EXPR && verifyFunctionCallee(node.callee, functionName) && node.arguments !== undefined && node.arguments.length > argumentIndex) { const arg = node.arguments[argumentIndex]; + // No need to localize empty strings. + if (arg.type == 'Literal' && arg.value === '') + return; + if (!isLocalizationCall(arg)) { let order = ''; switch (argumentIndex) { @@ -213,13 +246,13 @@ function checkFunctionArgument(functionName, argumentIndex, node, filePath, erro * Check esprima node object that represents the AST of code * to see if there is any localization error. */ -function analyzeNode(node, filePath, errors) { +function analyzeNode(parentNode, node, filePath, errors) { if (node === undefined || node === null) return; if (node instanceof Array) { for (const child of node) - analyzeNode(child, filePath, errors); + analyzeNode(node, child, filePath, errors); return; } @@ -260,7 +293,7 @@ function analyzeNode(node, filePath, errors) { break; default: // String concatenation to localization call(s) should be changed - checkConcatenation(node, filePath, errors); + checkConcatenation(parentNode, node, filePath, errors); // 3rd argument to createInput() should be localized checkFunctionArgument('createInput', 2, node, filePath, errors); break; @@ -268,7 +301,7 @@ function analyzeNode(node, filePath, errors) { for (const key of objKeys) { // recursively parse all the child nodes - analyzeNode(node[key], filePath, errors); + analyzeNode(node, node[key], filePath, errors); } } @@ -282,7 +315,7 @@ async function auditFileForLocalizability(filePath, errors) { const relativeFilePath = getRelativeFilePathFromSrc(filePath); for (const node of ast.body) - analyzeNode(node, relativeFilePath, errors); + analyzeNode(undefined, node, relativeFilePath, errors); } function shouldParseDirectory(directoryName) {